-
Notifications
You must be signed in to change notification settings - Fork 94
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
Replace stacking gradient search with resample_blocks variant #626
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 93.97% 93.93% -0.04%
==========================================
Files 86 86
Lines 13848 13494 -354
==========================================
- Hits 13013 12675 -338
+ Misses 835 819 -16
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of initial comments. I'll do some real-life testing next.
I'm surprised you could get rid of such a huge chunk of code!
if ((isinstance(source_geo_def, AreaDefinition) and isinstance(target_geo_def, AreaDefinition)) or | ||
(isinstance(source_geo_def, SwathDefinition) and isinstance(target_geo_def, AreaDefinition))): |
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.
Could these be is_area_to_swath(source_geo_def, target_geo_def)
and is_swath_to_area(source_geo_def, target_geo_def)
? Also, this bit seems to be untested, likely also originally.
@@ -618,7 +353,7 @@ def block_bilinear_interpolator(data, indices_xy, fill_value=np.nan, block_info= | |||
res = ((1 - weight_l) * (1 - weight_p) * data[..., l_start, p_start] + | |||
(1 - weight_l) * weight_p * data[..., l_start, p_end] + | |||
weight_l * (1 - weight_p) * data[..., l_end, p_start] + | |||
weight_l * weight_p * data[..., l_end, p_end]) | |||
weight_l * weight_p * data[..., l_end, p_end]).astype(data.dtype) |
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 make sure the calculation is does not promote the dtype
in the first place by making the weights the same type?
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.
I'm not sure. I think I want to keep the highest precision to do the bilinear interpolation, and only then convert to float32. But maybe I'm being too careful?
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.
I doubt there's anything lost whether the interpolation is done with float32
or float64
. Of course the weights need to be floats, so casting to integer dtypes would still need additional handling... But as a test, it might be interesting to see the difference in memory usage between f32/f64 weights.
Also, preliminary test, resampling that metop scene to euron1 takes 5GB on main vs 3GB with this PR |
This PR replaces the stacking gradient search with resample blocks for the swath to area case.
It also adds support for area to swath in the gradient search.