-
Notifications
You must be signed in to change notification settings - Fork 295
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
Enable to pass a custom function to Scene.aggregate #2497
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2497 +/- ##
==========================================
+ Coverage 94.83% 94.85% +0.01%
==========================================
Files 337 337
Lines 49430 49658 +228
==========================================
+ Hits 46875 47101 +226
- Misses 2555 2557 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Pull Request Test Coverage Report for Build 5346938525
💛 - Coveralls |
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.
Seems simple enough. I had one small change to request. Otherwise, I'm wondering if you could refactor the aggregate method into smaller sub-methods (or external functions) to make CodeScene happy?
Complementary note: this method fails if the source area is a SwathDefinition because the "resolution" can't be inferred if not present as attributes to lons/lats |
@mraspaud milestone |
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.
Thanks for adding this! just a couple of comments inline.
satpy/scene.py
Outdated
def _get_area_resolution(area): | ||
"""Attempt to retrieve resolution from AreaDefinition.""" | ||
try: | ||
resolution = max(area.pixel_size_x, area.pixel_size_y) | ||
except AttributeError: | ||
resolution = max(area.lats.resolution, area.lons.resolution) | ||
return resolution |
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.
This feels like it should already be present somewhere in pyresample. I think we have a method called geocentric_resolution
, can it be used here?
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 could do that but it would break the back compatibility !
For AreaDefinition for example, right now (and in the past) the method is/was using pixel_size (in units of projection coordinates). While geocentric_resolution
if I understand correctly would provides the geocentric resolution (in degrees)
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.
You are correct that it is not the same, but I do want to clarify it should be in meters...just not the same meters as the projection. I think, especially since you didn't write this code, that it can stay as is (again, my opinion). I do think the .resolution
should be .attrs["resolution"]
though. If you have other changes to make maybe you could do that too.
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.
@djhoese you mean the Scene DataArray .attrs['resolution']
? But is this always updated? Also when resampling to new area/swaths? And what if the attribute is missing? I guess we should discuss this on a separate issue and do a separate PR ...
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.
No no, sorry. I mean in the except clause it does area.lats.resolution
. If I'm understanding this code area
is a SwathDefinition
in this except
block, .lats
is a DataArray
, and area.lats.resolution
is a shortcut to area.lats.attrs["resolution"]
. The .attrs
form of accessing this information is the preferred way of doing it.
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.
Done and likely ready to be merged ;)
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.
LGTM
This small PR adds the option to pass a custom function to
Scene.aggregate
AUTHORS.md
if not there already