Store dataset properties in netCDF-safe attrs with backward-compatible reads, add CI test workflow, and include optional IFS dependencies#21
Conversation
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/2645ad1c-3573-48c9-88f0-5150eb23b01f Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/2645ad1c-3573-48c9-88f0-5150eb23b01f Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
| SPACE_ATTR = "properties_space" | ||
| TIME_ATTR = "properties_time" | ||
| UNCERTAINTY_ATTR = "properties_uncertainty" |
There was a problem hiding this comment.
Let us use "." as a separator instead of "_".
There was a problem hiding this comment.
Updated in 605b283. I switched the new properties attribute keys to use . separators (properties.space, properties.time, properties.uncertainty) and updated tests accordingly. I also kept read compatibility for previously introduced underscore flat keys.
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/77c180d8-40ee-44d4-b245-85a382fff6a4 Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
| LEGACY_SPACE_ATTR = "properties_space" | ||
| LEGACY_TIME_ATTR = "properties_time" | ||
| LEGACY_UNCERTAINTY_ATTR = "properties_uncertainty" |
There was a problem hiding this comment.
Don't add legacy support for these properties. They were introduced in this PR.
There was a problem hiding this comment.
Done in 9ece8b5. I removed the underscore flat-key fallback and constants, and deleted the underscore compatibility test so only dotted attrs plus pre-existing properties dict/JSON legacy reads remain. No UI changes in this PR.
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/7ea2a6c3-a3f2-4be2-8210-cabc0af65ae2 Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
observingClouds
left a comment
observingClouds
left a comment
There was a problem hiding this comment.
Loogs good, just some linting issues to fix. Please run the linters and fix accordingly.
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/3e3fab28-7e29-4f5e-b20d-c0ca122da9bd Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
Fixed in 3bdf9bf. I ran the repo linters via |
observingClouds
left a comment
observingClouds
left a comment
There was a problem hiding this comment.
add a CI action that actually runs the test
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/470afd36-0845-4b9b-864f-6e1a0e536c32 Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
Implemented in f596fe1. I added a new GitHub Actions workflow ( |
|
@copilot address requested changes |
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/e06d9499-d27d-47d7-b76f-4cb0470fc5d9 Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
|
@copilot add a changelog entry describing the changes made in this PR 21. |
Agent-Logs-Url: https://github.com/mlwp-tools/mxalign/sessions/ce510f28-d96f-401e-a6d7-602ccc1e9ede Co-authored-by: observingClouds <43613877+observingClouds@users.noreply.github.com>
Added in commit |
|
Any reason not to use pytest for CI testing? |
|
@mpvginde no particular reason. I changed it to |
|
I also don't have a favourite, but that way we are consistent with |
observingClouds
force-pushed
the
copilot/fix-properties-dataset-attribute
branch
from
33c27a4 to
fdc412c
Compare
This PR fixes #17 by storing the dataset properties as individual metadata attributes.
In addition, this PR adds a CI test to ensure the changes are intact. As mxalign now also depends on cfgrib and the eccodes library to load ifs forecasts those dependencies have been added as well.