WMO Instruments Part 2.1: Enhancements

[sfinkens]

Deprecate sensor enhancement attribute in favor of instruments. To be merged into pytroll#3390

Summary

• Change sensor: mysensor to instruments: [mysensor] in enhancement YAML files
• Update enhancement decision tree to support both sensor and instruments, with a deprecation warning for sensor
• Update decision tree to support "multival" tuples with a single element

Breaking Changes

None

• Closes #xxxx
• Tests added
• Fully documented
• Add your name to AUTHORS.md if not there already

[sfinkens]

@djhoese @mraspaud Please review (can't request that via UI because it's in my fork)

[coveralls]

Coverage Report for CI Build 26277325350

Coverage decreased (-0.004%) to 96.39%

Details

• Coverage decreased (-0.004%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• 11 coverage regressions across 2 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

11 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
enhancements/enhancer.py	10	91.87%
decision_tree.py	1	98.44%

---

Coverage Stats

Relevant Lines:	59081
Covered Lines:	56948
Line Coverage:	96.39%
Coverage Strength:	2.89 hits per line

---

💛 - Coveralls

[sfinkens]

I first tried to specify single instruments as a list in the YAML, but apparently that doesn't work. For example this YAML

  test1_sensor1_specific:
    name: test1
    instruments: [test_sensor1]  # instead of just "test_sensor1"
  test1_sensor2_specific:
    name: test1
    instruments: [test_sensor2]  # instead of just "test_sensor2"

would break test_multisensor_choice (wrong enhancement applied). Looks like the order of the elements in the tree is different

With strings:

name=test1
  reader=<wildcard>
    platform_name=<wildcard>
      instruments=test_sensor1
        standard_name=<wildcard>
          units=<wildcard>
            | name=test1
            | instruments=test_sensor1
      instruments=test_sensor2
        standard_name=<wildcard>
          units=<wildcard>
            | name=test1
            | instruments=test_sensor2

With lists:

name=test1
  reader=<wildcard>
    platform_name=<wildcard>
      instruments=('test_sensor2',)
        standard_name=<wildcard>
          units=<wildcard>
            | name=test1
            | instruments=['test_sensor2']
      instruments=('test_sensor1',)
        standard_name=<wildcard>
          units=<wildcard>
            | name=test1
            | instruments=['test_sensor1']

The DecisionTree docstring is pretty clear,

A multi-value key can be specified as a single value
(typically a string) or a set.

but the behaviour confused me a little bit.

[djhoese]

Ok so you're saying the alphabetical-ness of the sensors/instruments handling in the decision tree isn't working and that that test is failing?

[djhoese]

I wonder if this if statement needs to be updated to convert all scalar strings from the YAML into a sorted tuple:

satpy/satpy/decision_tree.py

Lines 166 to 167 in 8061698

	if match_key in self._multival_keys and isinstance(this_attr_val, list):
	this_attr_val = tuple(sorted(this_attr_val))

This, along with your forcing of .attrs["instruments"] to be a set should make this more consistent and easier to predict.

[djhoese]

Side note: This windows py3.11 environment is consistently hanging on some test. Some test around 93-95% it looks like.

[sfinkens]

Ok so you're saying the alphabetical-ness of the sensors/instruments handling in the decision tree isn't working and that that test is failing?

The test only fails if the YAML contains an instrument list with a single element. Strings work fine.

I wonder if this if statement needs to be updated to convert all scalar strings from the YAML into a sorted tuple

Thanks for the hint. That, together with a change to the query, made the test pass (mix of list and string would also work)

diff --git a/satpy/decision_tree.py b/satpy/decision_tree.py
index c1f5e2516..ed5f84bb5 100644
--- a/satpy/decision_tree.py
+++ b/satpy/decision_tree.py
@@ -163,8 +163,11 @@ class DecisionTree:
             for match_level, match_key in enumerate(self._match_keys):
                 # or None is necessary if they have empty strings
                 this_attr_val = sect_attrs.get(match_key, self.any_key) or None
-                if match_key in self._multival_keys and isinstance(this_attr_val, list):
-                    this_attr_val = tuple(sorted(this_attr_val))
+                if match_key in self._multival_keys:
+                    if isinstance(this_attr_val, list):
+                        this_attr_val = tuple(sorted(this_attr_val))
+                    elif isinstance(this_attr_val, str):
+                        this_attr_val = tuple([this_attr_val])
                 is_last_key = match_key == self._match_keys[-1]
                 level_needs_init = this_attr_val not in curr_level
                 if is_last_key:
@@ -179,7 +182,9 @@ class DecisionTree:
     @staticmethod
     def _convert_query_val_to_hashable(query_val):
         _sorted_query_val = sorted(query_val)
-        query_vals = [tuple(_sorted_query_val)] + _sorted_query_val
+        query_vals = [tuple(_sorted_query_val)]
+        query_vals += [tuple([v]) for v in _sorted_query_val]
+        query_vals += _sorted_query_val
         query_vals += query_val
         return query_vals
 
diff --git a/satpy/tests/enhancement_tests/test_enhancer.py b/satpy/tests/enhancement_tests/test_enhancer.py
index bcb150405..06e03da02 100644
--- a/satpy/tests/enhancement_tests/test_enhancer.py
+++ b/satpy/tests/enhancement_tests/test_enhancer.py
@@ -144,7 +144,7 @@ class TestComplexSensorEnhancerConfigs(_BaseCustomEnhancementConfigTests):
 enhancements:
   test1_sensor1_specific:
     name: test1
-    instruments: test_sensor1
+    instruments: [test_sensor1]
     operations:
     - name: stretch
       method: !!python/name:satpy.enhancements.contrast.stretch
@@ -160,7 +160,7 @@ enhancements:
       kwargs: {stretch: crude, min_stretch: 0, max_stretch: 100}
   test1_sensor2_specific:
     name: test1
-    instruments: test_sensor2
+    instruments: [test_sensor2]
     operations:
     - name: stretch
       method: !!python/name:satpy.enhancements.contrast.stretch

This is pretty unknown terrain for me and I don't understand all the implications of this change. Do you think it is reasonable and worth including? (together with updating all the enhancement YAMLs with instrument lists)

[djhoese]

I don't think the YAMLs have to be changed beyond the name of that key. I think it makes sense and is convenient for users to be able to list a single string for their sensor (instruments: abi) but then again maybe since the key name is plural it makes sense to require the matching criteria to also be a list/set and never a scalar string. "You have 1 instruments" is not proper grammar 😉

[sfinkens]

I prefer proper grammar 😀

[djhoese]

I don't remember how this worked in the old code so I don't understand how this works now, but if you say it works then 👍

[sfinkens]

I've updated the docstring to make it more clear.

[djhoese]

Just some small comments but I think this is good. So this would be part of Satpy 1.0?

[sfinkens]

So this would be part of Satpy 1.0?

Yes, but together with the other WMO parts

[sfinkens]

I've also wrapped the compatibility check with 8< v1.0 8< v1.1 scissors

[djhoese]

Oh so this could actually be merged since it has the backwards compatibility and the scissor cut outs for 1.0? I thought this would be a merge after the last pre-1.0 release, but before 1.0. If it works in the pre-1.0 release then that's great.

[sfinkens]

Yes I'm just updating all the WMO PRs so that they can be included the last pre-1.0 release