Fix derivation of correct radius of influence when data layout is not standard#555
Conversation
…e 'y' dimension for rows/scanlines This makes the code resillient towards any other (awkward) data layout than the standard (where first dimension is usually the rows. Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
|
It looks like I can assume that |
For Satpy, yes, but we can't (and shouldn't) make that assumption for pyresample. |
Ok, got me there then! I was looking for tests on that, but found only with xarray, but possible that I just overlooked such tests... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #555 +/- ##
==========================================
+ Coverage 94.11% 94.14% +0.03%
==========================================
Files 82 84 +2
Lines 13078 13199 +121
==========================================
+ Hits 12308 12426 +118
- Misses 770 773 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
djhoese
left a comment
djhoese
left a comment
There was a problem hiding this comment.
Please review my comment on the issue you made:
pyresample/geometry.py
Outdated
| from typing import Optional, Sequence, Union | ||
|
|
||
| import numpy as np | ||
| import xarray as xr |
There was a problem hiding this comment.
This makes xarray a hard requirement on pyresample which it is not. Plus DataArray is already imported in a try/except below.
pyresample/geometry.py
Outdated
| # Data have no information on dimensions, so we assume first dimension (the rows) is the y-axis: | ||
| logger.warning('As Numpy data arrays carry no information on the data layout we here ' + | ||
| 'assume the first dimension (the rows) is the y-axis (the satellite scans)') | ||
| rows = self.shape[0] |
There was a problem hiding this comment.
So users get a warning for every execution of this method? No, we can't do this.
There was a problem hiding this comment.
warnings.warn would in theory only issue a warning on the first execution.
There was a problem hiding this comment.
Lol right, sure, but the overall point still stands. I would still prefer that users receive no warning for a usage that has existed for years and years and is perfectly fine in 99.9999% of cases that we run into.
My preference is still that the Satpy reader reorient data, but I also have no experience with that instrument.
There was a problem hiding this comment.
Ok I take back the "years and years" thing, I thought this was a different method being modified...but still.
There was a problem hiding this comment.
I agree that in this particular case, there should be no warning at all.
There was a problem hiding this comment.
Ha, very good, I did trigger a reaction here! I was pretty sure you wouldn't like this when I wrote it, but wanted to discuss it. Done now. So sorry, I take it back again.
There was a problem hiding this comment.
Ha, very good, I did trigger a reaction here! I was pretty sure you wouldn't like this when I wrote it, but wanted to discuss it. Done now. So sorry, I take it back again.
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
|
@djhoese I made a comment in the issue: #554 (comment) Which solution do you propose I pursue? And does @gerritholl and @mraspaud have opinions as well? |
4 participants
This is supposed to solve #554
Make use of the fact that longitudes are an xarray data array and use 'y' dimension for rows/scanlines
This makes the code resilient towards any other (awkward) data layout than the standard (where first dimension is usually the rows).
git diff origin/main **/*py | flake8 --diff