Fix dtype for swath -> area resampling with gradient search #618
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #618 +/- ##
=======================================
Coverage 93.97% 93.98%
=======================================
Files 86 86
Lines 13825 13839 +14
=======================================
+ Hits 12992 13006 +14
Misses 833 833
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Performance comparisons with my 12 AVHRR composites: Satpy Satpy pytroll/satpy#2893 , Pyresample Satpy pytroll/satpy#2893 , Pyresample: this PR So from 5.6 GB to 3.5 GB of RAM and from 87.3 s to 68.6 s processing time. |
pyresample/gradient/__init__.py
Outdated
| @@ -421,7 +421,7 @@ def parallel_gradient_search(data, src_x, src_y, dst_x, dst_y, | |||
| dst_x[i], dst_y[i], | |||
| method=method) | |||
| res = da.from_delayed(res, (num_bands, ) + dst_x[i].shape, | |||
| dtype=np.float64) | |||
| dtype=np.float64).astype(arr.dtype) | |||
There was a problem hiding this comment.
Ok, I think we can do better than this. What I see here (and a bit above) is that we convert the input data to float64, do the resampling, then convert back to float32.
Using cython fused types, we should be able to template the cython function to accept both float64, float32, or even ints. This cython doc section should help. I can give you a hand if you don't feel comfortable with this.
There was a problem hiding this comment.
Yeah, I'm not going to learn Cython and its typing that deeply at this point 😅
|
@pnuu I pushed a couple of commits that introduce fused types in the cython so that both float64 and float32 input data is allowed and handled correctly in principle. |
|
@mraspaud definitely a better solution! I don't see a difference in the performance though: |
|
@pnuu did you recompile the cython code? |
|
Yeah, I ran |
|
After a discussion on slack, it was noted that float32 is not precise enough for cartesian coordinates at least, so I removed the fused type for coordinates, only allowing float64 (double in cython) |
|
@pnuu where are we on this one? |
|
I think we're good and ready to merge. The RAM footpring nor processing time didn't change with the coordinate dtype de-fusing from the previous graph above. All the datasets stay as they were, for example with pytroll/satpy#2893 the datasets/composites stay |
|
@djhoese would you mind giving your review on this? |
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
|
Thanks for the updates and tweaks @mraspaud! |
|
Yikes, the resulting image has transposed!
|
|
Apparently this is the case also with
|
What??? |
|
@mraspaud this seems to be a problem only with polar data (AVHRR L1b in my test case), so |
|
@pnuu ok, if this a problem also in main, can we merge this and fix the other issue in another PR? |
|
@mraspaud yes, I'm fine with that 👍 |
Resampling
float32swath data resulted infloat64output. This is a simple fix for this data type promotion.