-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactoring steps-nowcast #436
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #436 +/- ##
==========================================
+ Coverage 83.89% 83.99% +0.10%
==========================================
Files 160 160
Lines 12902 13005 +103
==========================================
+ Hits 10824 10924 +100
- Misses 2078 2081 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
hi @sidekock , first of all, thanks for taking up this heroic task :-)
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 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. |
@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. |
@sidekock, I will try to do so asap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
self._print_forecast_info() | ||
# Measure time for initialization | ||
if self.measure_time: | ||
self.start_time_init = time.time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test where measure_time is True (and False)? That would probably tick the box for the test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could to this but I don't know if it is worth it, its an internal method with almost no functionality...
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? |
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