Add 'preference' option to 'get_satpos' utility #2030
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2030 +/- ##
==========================================
- Coverage 93.65% 93.63% -0.02%
==========================================
Files 282 282
Lines 41878 41880 +2
==========================================
- Hits 39220 39214 -6
- Misses 2658 2666 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| def get_satpos(dataset): | ||
| def get_satpos( | ||
| data_arr: xr.DataArray, | ||
| preference: Optional[str] = None, |
There was a problem hiding this comment.
Could this preference be also fetched from the central satpy configuration if not provided?
There was a problem hiding this comment.
Yes! Sorry I missed this. I commented on slack. This is exactly the idea I had, but I think the preference should be use case specific. Meaning, the config option would be for controlling sensor angle generation and how it uses this versus a global/generic "whenever satellite position is requested prefer 'X'".
There was a problem hiding this comment.
I still think there should be a central config for this that can be undefined by default, and still allow this flag to be present. But this is good enough for now.
There was a problem hiding this comment.
I'm not completely against this idea, but I'm not sure what purpose it serves. Users don't typically use get_satpos, only other Satpy components do. Those satpy components (ex. angle generation) should be the things with the config options for the preference.
There was a problem hiding this comment.
As discussed on slack, what @mraspaud is going discussing something like a group of configuration parameters or a "profile" where you can say satpy.config.set_group("image_optimizations") and the various time or satellite positions or other parameters would be set to specific values that would be allow for the fastest generation of images. Of course I just made up the name for that parameter group, but hopefully the idea is clear. This would be a big feature and is not part of this PR, but I wanted to mention it here as something that would be affected by this type of configuration grouping.
There was a problem hiding this comment.
Nice work, that's a very useful feature! Two comments:
- Looks like this drops support for legacy attributes (
satellite_longitude/latitude/altitude). However, there might be some readers around that still provide only the legacy attributes. I know you asked about that but I thought this was about AHI HSD only. - If none of the preferred keys are available and also the fallback is missing, this will fail with a plain KeyError (not your fault, that's the current behaviour). If you have time, I think you could copy the following block from Improve error reporting of get_satpos #1925:
try:
return _get_satpos(dataset)
except KeyError:
warnings.warn('Failed to get satellite position from orbital '
'parameters, using legacy attributes instead.')
try:
return _get_satpos_legacy(dataset)
except KeyError:
raise KeyError("Unable to determine satellite position. Either"
"the reader doesn't provide that information or"
"geolocation datasets are not available.")
|
|
@sfinkens So the only two readers I found that use the legacy keys and don't also have |
|
@sfinkens Your concerns have been addressed (at least I think so). In order to continue work on other things I'm going to merge this, but if you have any additional issues let me know and I'll make them. |
|
Thanks for removing the legacy position! |
As part of the discussion on slack regarding AHI HSD performance and optimizations I've created this PR to allow modifiers and other users hoping to get satellite position information to have control over what precision or quality of data they prefer. This will be useful in the AHI HSD case as the reader can be updated to include a "nominal" satellite position and (hopefully) share calculations of things like sensor zenith/azimuth angles because the satellite position will be the same between bands rather than the "actual" position which differs between bands and even differs between segments of a band.
Related #2029 #2012