-
Notifications
You must be signed in to change notification settings - Fork 321
Add config switch for OSCAR compliant attributes #3291
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3291 +/- ##
==========================================
- Coverage 96.30% 96.29% -0.02%
==========================================
Files 463 463
Lines 58863 58899 +36
==========================================
+ Hits 56689 56716 +27
- Misses 2174 2183 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 19424398694Details
💛 - Coveralls |
|
Is failing CI expected at this point? |
No, locally these tests pass. I tried merging main but that didn't help. |
|
Seemed like Pyspectral LUT download had failed, again. I restarted the builds. |
Add a config switch for toggling OSCAR compliant attributes (
platform_name,sensor) and adapt ABI readers to see what happens.The ABI modifications should probably be reverted and applied together with all readers once we've agreed on the way forward.
Deprecation Strategy
Changing platform/instrument names has a high potential of breaking users' workflows because they are often used in filenames. So we decided to add a config switch for toggling OSCAR compliant attributes. With satpy-1.0 this would default to True, but we keep it for a couple of releases so that users can get the legacy attributes back if they need more time to adapt.
Implementation
FileYAMLReaderrevert them back to legacy attributes if desired.dependenciesattribute in the composites definition. So far dependencies were specified using forward slashessensor_name: dep1/dep2/sensor. But since OSCAR sensor names can contain slashes (AVHRR/1) this doesn't work anymore.8< v1.0sciccors so that they can be removed using a script.Notes:
Upstream Changes
Satpy passes dataset attributes to pyspectral which internally works with OSCAR names, but expects lowercase names in the interface. So that needs to be updated/deprecated as well. Ideally Satpy would also set the pyspectral config switch. For the moment I just lowered the sensor name before passing it to pyspectral.
Failed Approaches
scene.sensor_nameswould contain bothABIandabi.sensorandplatform_nameproperties to the file handlers. Problem: A file may contain data from multiple sensors and sometimes the sensor is only known after loading the dataset.Class Diagram
--- config: theme: 'default' --- classDiagram class Scene { +sensor_names [ABI] +available_composite_names() } class Reader { +sensor_names [ABI] } class ReaderYAML["readers/abi_l1b.yaml"] { sensors: [ABI] } class DataArray { +sensor ABI +platform_name GOES-18 } class CompositesYAML["composites/abi.yaml"] { sensor_name: ABI dependencies: [visir] 🆕 } class LoadComp["config_loader.py"] { +load_compositor_configs_for_sensor() +sensor_to_filename() ABI -> abi.yaml 🆕 } Scene--> Reader : Collects sensor names from Scene --> DataArray : Collects sensor names from Scene --> LoadComp : Passes sensor names to Reader --> ReaderYAML : Reads sensors from LoadComp --> CompositesYAML : Reads composites & dependencies fromsensor.lower()hack