Add config switch for OSCAR compliant attributes

[sfinkens]

Add a config switch for toggling OSCAR compliant attributes (platform_name, sensor) and adapt ABI readers to see what happens.

with satpy.config.set(oscar_compliant_attributes=True):
    scene = Scene(...)

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

• Change all file handlers to provide OSCAR compliant dataset attributes. In FileYAMLReader revert them back to legacy attributes if desired.
• Update platform/sensor name in yaml definitions (readers, composites, enhancements), but keep the filenames as they are. This requires two additions
  1. Translation from OSCAR sensor name to Satpy filename.
  2. A new dependencies attribute in the composites definition. So far dependencies were specified using forward slashes sensor_name: dep1/dep2/sensor. But since OSCAR sensor names can contain slashes (AVHRR/1) this doesn't work anymore.
• Mark changes with 8< v1.0 sciccors so that they can be removed using a script.

Notes:

• As far as I can see, composites get their sensor attribute from dataset attributes and not from yaml definitions. So the config switch would also work for composites.

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

• Use OSCAR attributes only in dataset attributes and keep yaml files as they are. Problem: scene.sensor_names would contain both ABI and abi.
• Adding sensor and platform_name properties 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 from

Loading

• Prepares for Standardize platform and sensor names #885
• Wait for upstream changes in pyspectral
• Remove sensor.lower() hack
• Remove ABI modifications
• Tests added
• Fully documented

[codecov]

Codecov Report

❌ Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.34%. Comparing base (4f0befb) to head (fbf6827).
⚠️ Report is 157 commits behind head on main.

Files with missing lines	Patch %	Lines
satpy/readers/core/yaml_reader.py	94.87%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3291      +/-   ##
==========================================
+ Coverage   96.30%   96.34%   +0.03%     
==========================================
  Files         463      466       +3     
  Lines       58863    59132     +269     
==========================================
+ Hits        56689    56970     +281     
+ Misses       2174     2162      -12     

Flag	Coverage Δ
behaviourtests	3.62% <37.50%> (+0.01%)	⬆️
unittests	96.43% <96.42%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coveralls]

Pull Request Test Coverage Report for Build 19424398694

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall first build on oscar-compliant-attrs at 96.383%

---

Totals
Change from base Build 19293346095:	96.4%
Covered Lines:	56602
Relevant Lines:	58726

---

💛 - Coveralls

[djhoese]

Is failing CI expected at this point?

[sfinkens]

Is failing CI expected at this point?

No, locally these tests pass. I tried merging main but that didn't help.

[pnuu]

Seemed like Pyspectral LUT download had failed, again. I restarted the builds.

[mraspaud]

Sorry it took so long for me to have a proper look at this.
I think it looks really nice, and I would vote for going forward with this.

Just one small thing: Would it be possible to have a small private function (eg _convert_to_pyspectral_name) that does the conversion to pyspectral names? It would just do .lower() for now, but when pyspectral is updated it will make it easier to find the place where to implement the transition.

[sfinkens]

Would it be possible to have a small private function (eg _convert_to_pyspectral_name)

Yes, good idea!

[sfinkens]

I've added the pyspectral conversion helper, turned the config switch off for now and removed the ABI modifications. In the next PR I would then update all the readers.

[djhoese]

Tests are failing...I assume (hope) that's due to your changes.

[sfinkens]

Tests are failing...

Yes, some readers provide sensor as set type. I've taken this into account for now. Since the sets actually contain only one sensor, it might make sense to update those readers v1.0 to return a string as well.

[djhoese]

It is perfectly valid though for readers to return a set for sensors. Obviously a set with a single item is unnecessary, but it should still be supported in my opinion. Some readers provide data for multiple sensors so the set support is needed anyway.

[sfinkens]

Hmm, but what if the dataset has been loaded? Can this be multi-sensor, too? I'm just referring to the dataset attribute sensor here. And if it is multi-sensor, how can this information be passed to pyspectral?

[sfinkens]

Summary from slack discussion: It is possible for higher levels of processing where data from multiple sensors is used to generate a product. Those won't be passed to pyspectral, though.

Although the set/str mix is not perfectly consistent, storing all sensor attributes as a set is probably confusing new users/contributors and also breaks even more workflows than changing to WMO names.