WMO Instruments Part 1#3390
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3390 +/- ##
==========================================
- Coverage 96.34% 96.32% -0.02%
==========================================
Files 466 468 +2
Lines 59155 59164 +9
==========================================
Hits 56990 56990
- Misses 2165 2174 +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:
|
|
Would it make sense to move those instrument helper methods to a dedicated module |
updates: - [github.com/astral-sh/ruff-pre-commit: v0.15.9 → v0.15.12](astral-sh/ruff-pre-commit@v0.15.9...v0.15.12) - [github.com/pre-commit/mirrors-mypy: v1.20.0 → v1.20.2](pre-commit/mirrors-mypy@v1.20.0...v1.20.2) - [github.com/pycqa/isort: 8.0.1 → 9.0.0a3](PyCQA/isort@8.0.1...9.0.0a3)
djhoese
added
enhancement
|
I added the "backwards-incompatibility" label on this PR. Let me know if that isn't accurate. You should rebase this PR on |
djhoese
left a comment
djhoese
left a comment
There was a problem hiding this comment.
Looks pretty good. I had a couple questions inline. I think my biggest long term fear after seeing these changes is handling the "sensor" in the enhancement decision tree.
| "reader", | ||
| "platform_name", | ||
| "sensor", | ||
| instr_key, |
There was a problem hiding this comment.
Oh...this is difficult. I wonder if we should have a deprecation cycle or this? Or accept both and display a warning if "sensor" is detected/used?
There was a problem hiding this comment.
Ah, this is where the sensor key from enhancement yamls is actually used? And apparently users can provide their own enhancements, so I think accepting both + warning is a good idea 👍
There was a problem hiding this comment.
That makes me think if we should do that everywhere. For example when checking attributes
def get_instruments_from_attrs(attrs):
# simplified a bit
legacy = attrs.get("sensor", {})
if legacy:
warnings.warn(...)
return attrs.get("instruments", legacy)and in the yaml reader.
There was a problem hiding this comment.
Oh maybe. Like if the user is creating their own DataArray (either "by hand" or in custom compositor code)? That makes sense.
And yes, this list is what is considered "identifying" information for an enhancement section in an enhancement YAML. It is used, in order, to build up a decision tree to choose which enhancement to use for a particular DataArray.
There was a problem hiding this comment.
I've reverted this change in the enhancement decision tree, because I think it fits better into PR2 (together with the yaml reader update). But I've updated get_instruments_from_attrs as described above.
…atpy into wmo-instruments-part1
Coverage Report for CI Build 25488772252Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage at 96.398% (no base build to compare)Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
I also moved the instrument helpers to their own module. |
| moptions = moptions.copy() | ||
| moptions.update(comp_id.to_dict()) | ||
| moptions["sensor"] = sensor_name | ||
| moptions[instr_key] = sensor_name |
There was a problem hiding this comment.
Here's another one. I first thought it was a keyword argument, but actualy it is added to dataset attributes here
3 participants
This is the first of 4 PRs for getting WMO instrument names.
instrumentsattribute and use that instead of
sensor.sensorattribute (probably after v1.0 has been released)I hope this way the changes are less painful to review. To achieve
that, helper methods were added for
instruments_keythat controls which attributeSatpy is using. This is mostly for postponing the file handler changes
to the second PR. Current default is
sensor, will be changed toinstrumentsonce all file handlers provide aninstrumentsattribute. Or maybe removed entirely.
Breaking Changes
To make the tests pass for the moment, the
sensorattribute is converted to a set inget_instruments_from_attrs. We don't want to have that in main and it will be removedin PR2. So I'll do PR2 against this branch.
Notes
It would be nice to have a custom Attributes class with getters
and setters for the instrument, but xarray converts it back to
plain dicts internally.
AUTHORS.mdif not there already