Performance fix #410
Performance fix #410
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #410 +/- ##
==========================================
+ Coverage 83.52% 83.88% +0.35%
==========================================
Files 159 160 +1
Lines 12575 12780 +205
==========================================
+ Hits 10503 10720 +217
+ Misses 2072 2060 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @gjm174 for this contribution! I was curious to get an idea of the performance improvement, do you have some numbers?
Also, I'm a bit skeptical on some of these changes: you removed copies of the data, which surely ensures a lower memory footprint. Are we sure these copies were not needed to avoid side effects later on in the code? All tests are green, which is good, but I wouldn't rely too much on our test suite to detect such unintended effects.
perhaps @RubenImhoff could also have a look since you worked more closely on the steps blending code?
| @@ -173,7 +173,7 @@ def extrapolate( | |||
|
|
|||
| if xy_coords is None: | |||
| x_values, y_values = np.meshgrid( | |||
| np.arange(velocity.shape[2]), np.arange(velocity.shape[1]) | |||
| np.arange(velocity.shape[2]), np.arange(velocity.shape[1]), copy=False | |||
There was a problem hiding this comment.
I can see how this provides better performance, but are we 100% sure that there are no side effects?
There was a problem hiding this comment.
perhaps @pulkkins as the original author of this code can jump in here and give us a feedback on whether those copies were intentional or not?
There was a problem hiding this comment.
Is it likely that the velocity shape changes? If not, I expect that the copy is not needed.
There was a problem hiding this comment.
I think not, since the np.arange(...) creates a new array, and it's this new array that is no longer copied. When copy=True, the reference to the brand-new array at some point goes out of scope and the memory will be garbage collected anyway. I think this just avoids unnecessarily creating and throwing away an intermediate array.
| ] | ||
| precip_cascade = np.stack(precip_cascade) | ||
|
|
||
| precip_cascade = np.stack( |
There was a problem hiding this comment.
In this new version, you use a repeated reference to the same sliced data, which is more memory efficient but can lead to unintended side effects if the data is modified later. The previous version used deep copies to ensure that modifications to one ensemble member do not affect the others. Did you consider such aspects?
There was a problem hiding this comment.
I expect that this is still fine here, as this takes places prior to the dask-based parellization of the ensemble members.
The main performance improvement occurs in the reading of NWP files. The other minor changes save approximately 6-7 seconds. The loading of the NWP files now takes only one-fifth of the time it did previously. I have ensured that no unintended effects will occur by not making copies. |
|
Very good thanks! I'll wait for a second opinion from @RubenImhoff and then I'm happy to merge this PR |
|
I will have a look at it after this weekend. Great work so far, @gjm174! |
There was a problem hiding this comment.
@gjm174, nice work! I left a few comments, but other than, it seems good to go. Nice work!
| ] | ||
| precip_cascade = np.stack(precip_cascade) | ||
|
|
||
| precip_cascade = np.stack( |
There was a problem hiding this comment.
I expect that this is still fine here, as this takes places prior to the dask-based parellization of the ensemble members.
There was a problem hiding this comment.
Talking about deep copies, should:
forecast_prev = precip_cascade
noise_prev = noise_cascade
get a deep copy?
| @@ -173,7 +173,7 @@ def extrapolate( | |||
|
|
|||
| if xy_coords is None: | |||
| x_values, y_values = np.meshgrid( | |||
| np.arange(velocity.shape[2]), np.arange(velocity.shape[1]) | |||
| np.arange(velocity.shape[2]), np.arange(velocity.shape[1]), copy=False | |||
There was a problem hiding this comment.
Is it likely that the velocity shape changes? If not, I expect that the copy is not needed.
After profiling some changes which improve the overall performance, especially the loading of NWP files.