Refactor steps in blending code

[sidekock]

See #443

[codecov]

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.31%. Comparing base (a7dae54) to head (5fad664).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pysteps/nowcasts/steps.py	72.22%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
+ Coverage   84.26%   84.31%   +0.05%     
==========================================
  Files         160      160              
  Lines       13067    13251     +184     
==========================================
+ Hits        11011    11173     +162     
- Misses       2056     2078      +22     

Flag	Coverage Δ
unit_tests	84.31% <72.22%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sidekock]

Both visual tests and assert statements give the same conclusion: the output is exactly the same!

[sidekock]

@dnerini The codecov check is failing but the only things it is actuary failing on in the deepcopys I have done in the code and on the timing which does not seem worth it to write tests for as this is a very basic subtraction. Would it be able to disable it for these things or how does this work? I know you where able to do it for the re-factoring of the steps nowcasting.

[RubenImhoff]

Hi @sidekock and @mats-knmi,

Thanks a lot for your work here! I'm sorry I was not around the past days to answer some questions and think along. However, this looks great! With the large number of changes, it is becoming difficult to check everything thoroughly, so at least I'm happy to see that it gives exactly the same results. I think a to do for a new PR could be updating the documentation, but that is probably easier and cleaner once this PR is pushed.

[RubenImhoff]

Is this TODO still necessary? I can imagine we'd like to have this done prior to merging this PR.

[sidekock]

The two versions do now provide identical output. I just want to make sure this is the case for all configurations, meaning:

• Signle core single member
• Single core multiple member
• Multiple core single member
• Multiple core multiple members (less cores than members)
• Multiple core multiple members (more cores than members)

The reason I want to do this is that I discovered that the pysteps tests do not properly handle this (all tests passed in a previous version of the code but it crashed on issues related to number of cores and number of members).
If you also propose other tests, I would love to hear suggestions!
The tests would exist of running both the master and refactored branch and then doing the following checks:

• DataArray.identical(other) @mats-knmi do you know if this actually checks the values of the nowcast or only the structure and metadata. Its a bit unclear to me but since it is very fast, I would assume it does not test the data itself.
• Depending on the previous answer, I could also check some random fields of random times and members.
• Lastly I can do a visual inspection but if the previous two are fine I do not think this matter but it's a matter of having enough confidence before I actually make this the default code :)

[mats-knmi]

Identical should do an element wise comparison (at least that is what I gather from the docs). The docs say that identical is the same as equals but also checks metadata. The docs for equals say that it compares not only the structure but also the contents of the datasets. Doing a very simple test I do seem to be able to confirm this.

>>> xr.Dataset(data_vars={"x": ("x", np.array([1,2,3,4], dtype=np.float64)), "y": ("x", np.array([4,3,2,1], dtype=np.float64))}).identical(xr.Dataset(data_vars={"x": ("x", np.array([1,2,3,4], dtype=np.float64)), "y": ("x", np.array([4,3,2,1], dtype=np.float64))}))
True
>>> xr.Dataset(data_vars={"x": ("x", np.array([1,2,3,4], dtype=np.float64)), "y": ("x", np.array([4,3,2,1], dtype=np.float64))}).identical(xr.Dataset(data_vars={"x": ("x", np.array([1,2,3,4], dtype=np.float64)), "y": ("x", np.array([4,3,2,2], dtype=np.float64))}))
False

[sidekock]

Thanks, I should have done such a test myself but totally forgot. Ill do the last checks in the next few days and then we can close this 6 -month-work-in-progress :)

[dnerini]

@dnerini The codecov check is failing but the only things it is actuary failing on in the deepcopys I have done in the code and on the timing which does not seem worth it to write tests for as this is a very basic subtraction. Would it be able to disable it for these things or how does this work? I know you where able to do it for the re-factoring of the steps nowcasting.

@sidekock we can simply ignore the failing check and merge whenever you feel it's ready

[sidekock]

Results of my test on belgian NWP blending:

• Single core single member ✔️
• Single core multiple member ✔️
• Multiple core single member ✔️
• Multiple core multiple members (less cores than members) ✔️
• Multiple core multiple members (more cores than members) ✔️
• Test with longer lead time (6 hours with 5min resolution, previous ones where all 1hour) ✔️

All tests seem to pass. I will run all the pysteps tests one more time and if they all pass I think it is time to release this into the wild :)

[sidekock]

@RubenImhoff @mats-knmi all tests are still succeeding and all the extra tests that I have run say the two datasets are identical. Seems we can Squash and merge or do you think more tests need to be done?

[mats-knmi]

Sounds good, I don't have anything else, so squash and merge away!

[RubenImhoff]

Same for me, fantastic work! :)

[sidekock]

@dnerini Do I also have to bump the version or is this something I leave up to you? Or is this not something that is done for code refactoring?

[mats-knmi]

I would like a verion bump, even if it is just for the reproducibility with the RNG that I merged earlier, since that would help me out with the tests that we have running.

[sidekock]

I would like a verion bump, even if it is just for the reproducibility with the RNG that I merged earlier, since that would help me out with the tests that we have running.

Doing some last changes to the documentation and then ill be on that to make a last commit before I merge

[dnerini]

Fantastic to see this merged! Congratulations for all the hard work you put into this, well done!

[sidekock]

Fantastic to see this merged! Congratulations for all the hard work you put into this, well done!

@dnerini Thanks! The tests for the docs to seem to be failing due to the codecov:

Also the version does not seem to be bumped. Don't know what I did wrong because I changed the version in the PKG-info

[dnerini]

@sidekock the error while compiling the docs was caused by a deprecation in scipy (see #455), while the version needed to be bumped also in the setup.py (5ede008).

now it should be all good, shall we release v1.14?

[sidekock]

Yes, please! Then I can finally cross this one on my to-do list :)