Refactoring steps-nowcast

[sidekock]

I started with the steps nowcast because that seemed more straightforward to tackle first. Since both methods should look alike, they must both be changed. I can't figure out how to run the test locally, so I will have to wait and see what the pull request test gives me as feedback.

This is a first suggestion how to split up the code but look forward to suggestions and improvments

[codecov]

Codecov Report

Attention: Patch coverage is 86.05898% with 52 lines in your changes missing coverage. Please review.

Project coverage is 84.03%. Comparing base (0f859c1) to head (663a9a2).
Report is 1 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   83.89%   84.03%   +0.13%     
==========================================
  Files         160      160              
  Lines       12902    13031     +129     
==========================================
+ Hits        10824    10950     +126     
- Misses       2078     2081       +3     

Flag	Coverage Δ
unit_tests	84.03% <86.05%> (+0.13%)	⬆️

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]

I have a question about the Codacy Static Code Analysis. It sais the method _check_inputs is to complex while this has barely been touched since the last version, I moved a few extra lines here because that just makes more sense. Should I split up that part of the code to remove the error or not?

[dnerini]

hi @sidekock , first of all, thanks for taking up this heroic task :-)

can't figure out how to run the test locally

which part? to run the test locally you need to setup the test data correctly, see https://pysteps.readthedocs.io/en/stable/user_guide/example_data.html#example-data and https://pysteps.readthedocs.io/en/stable/developer_guide/test_pysteps.html

I have a question about the Codacy Static Code Analysis. It sais the method _check_inputs is to complex while this has barely been touched since the last version, I moved a few extra lines here because that just makes more sense. Should I split up that part of the code to remove the error or not?

I agree with you, _check_inputs seems well in line with the complexity of the other methods, and feels like breaking it down into smaller pieces would somewhat overcrow your class. I silenced this issue on codacy.

[sidekock]

@mats-knmi @RubenImhoff Could you take a look at this and give feedback? That way I can go on to the blending version. I don't want to start working on that one if this one is not in a finalized state.

[RubenImhoff]

@sidekock, I will try to do so asap.

[RubenImhoff]

Hi @sidekock, super nice work! Thanks a lot for picking up this tremendous task. I like that we use a class now, it is much cleaner this way.

I think all looks good, but I've made some suggestions to change some variable names to a longer name.

In addition, perhaps utils.nowcast_main_loop also can use a refactor based on the refactor here. In this nowcast_main_loop, the subtimesteps are also used. I remember that we were discussing about this during the hackathon. Perhaps this is the time to clean that up a bit, too? I might be the most difficult/unreadable part of the code.

[sidekock]

Hi @sidekock, super nice work! Thanks a lot for picking up this tremendous task. I like that we use a class now, it is much cleaner this way.

I think all looks good, but I've made some suggestions to change some variable names to a longer name.

In addition, perhaps utils.nowcast_main_loop also can use a refactor based on the refactor here. In this nowcast_main_loop, the subtimesteps are also used. I remember that we were discussing about this during the hackathon. Perhaps this is the time to clean that up a bit, too? I might be the most difficult/unreadable part of the code.

I agree this might be worth looking at but the thing I am a bit worried about is the that this loop is used for all nowcasters so minor changes could affect the code in a lot of different places. I was thinking to tackle the loop in the blending first (since that only affect one place) and then transfer what I learned back. Its a bit of double work but it seems easier to debug that way what works and does not work. Any feedback on the way I split up the code?

[dnerini]

@sidekock Should this be merged?

[mats-knmi]

I am sorry for only commenting now. I have been busy with some other stuff. I will go over the changes and leave some input where I have it. If you don't want to hold off merging, that's fine since Ruben already reviewed this. Maybe you can then take my input along in the next steps of the refactoring

[mats-knmi]

I have left my comments, let me know what you think. Again, sorry for taking so long to get back to you. Overall the code looks much more manageable now, compared to what it looked like before.

[sidekock]

@sidekock Should this be merged?

Let me use the feedback of Mats. I will contact you once it is ready to be merged.

[sidekock]

@mats-knmi could you look through the changes to give some feedback? I will look at the 'private' variables next week if I have time. In general, I think this approach is much cleaner compared to what we had before. We see what is changed and what type it is (state or param) and what is loaded (config, state or param) for each method.

For the 'private' discussion, I have taken a small look at the StackOverflow page but do not yet see how I would use this at this moment. Most variables in the code are part of a dataclass so can I still make then private then?

[mats-knmi]

Looks good, I added 2 small comments on the type hinting, but other than that I think this makes it a lot nicer indeed. Regarding the private discussion:
Every method that currently starts with an underscore could start with 2 underscores and then they could still be called from within the class, but not as easily from outside. (e.g. _nowcast_main could be renamed to __nowcast_main and then be called normally as self.__nowcast_main()).
Every variable that is defined in __init__ could be defined with 2 underscores and then still accessed normally within the class, but not as easily from outside. (e.g. self.__state = StepsNowcasterState() and then acccess it later like self.__state.precip_forecast)

[sidekock]

Looks good, I added 2 small comments on the type hinting, but other than that I think this makes it a lot nicer indeed. Regarding the private discussion: Every method that currently starts with an underscore could start with 2 underscores and then they could still be called from within the class, but not as easily from outside. (e.g. _nowcast_main could be renamed to __nowcast_main and then be called normally as self.__nowcast_main()). Every variable that is defined in __init__ could be defined with 2 underscores and then still accessed normally within the class, but not as easily from outside. (e.g. self.__state = StepsNowcasterState() and then acccess it later like self.__state.precip_forecast)

Thanks for all the feedback @mats-knmi and @RubenImhoff. The new version of the code feels much more readable now! I made the last changes and will soon start on the real challenge: the blending code :)

[sidekock]

@dnerini the Static code analysis is complaining again because I changed the name of the method "__check_inputs". Would it be possible to disable this once more? That way, all tests should pass now

[mats-knmi]

@sidekock I resolved all my comments that are fully resolved now, I left 2 open, since those are not fully resolved yet.

I think the docstring comment is important to still address, but the one about passing precip and velocity and/or timesteps not at the __init__ but at the compute_forecast is more of a stylistic preference.

[sidekock]

@sidekock I resolved all my comments that are fully resolved now, I left 2 open, since those are not fully resolved yet.

I think the docstring comment is important to still address, but the one about passing precip and velocity and/or timesteps not at the __init__ but at the compute_forecast is more of a stylistic preference.

The new naming is now applied and the documentation is provided.

[sidekock]

@mats-knmi if all the requested changes are present, would you please approve the changes? Then I know I can start working on the blending part after the merge

[mats-knmi]

Yes they are approved. I wanted to approve them in github, but I can't find the button to do this. Not sure if I am just blind or if I am not authorized to approve stuff... Nevermind I found the button. It is in a completely different place as to where I am used to in gitlab.

[sidekock]

@dnerini I think this is ready to be merged with the master branch

[dnerini]

Fantastic work! 🙌