feat(ilamb): emit kind, clean units and scalar reference identity#777
Merged
Conversation
Reference series now carry kind="reference" and drop the "Reference" source_id sentinel; model series keep both source_id and reference_source_id. Presentation units are normalised to a clean CF/UDUNITS form so a model series and its reference agree, and the typed presentation fields (value_units, value_long_name, index_units, calendar) are populated. Scalars carry reference_source_id and kind in their json_structure.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
✅ Files skipped from review due to trivial changes (25)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR updates ILAMB series metadata handling to normalise units, add ChangesCore standard.py changes and unit tests
Regression test-data fixture regeneration
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip6/regression/series.json (1)
3-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
kind: "model"to these CESM2 trace fixtures.These two model series still serialise
source_idandreference_source_idbut never emit the new top-levelkind, while the reference traces below already do. That leaves the re-minted baseline out of sync with the shared metric-value contract and it won’t catch a missing-model-kind regression. Please regenerate after the model traces also serialisekind: "model".Also applies to: 238-240, 460-466, 695-697
packages/climate-ref-ilamb/tests/test-data/emp-gleamgpcp2.3/cmip6/regression/series.json (1)
3-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
kind: "model"to these CanESM5 trace fixtures.Both model series here still omit the new top-level
kindeven though the reference traces further down now emitkind: "reference". As committed, this baseline won’t enforce the model-trace side of the shared metric-value contract. Please regenerate after the model traces also serialisekind: "model".Also applies to: 202-204, 388-403, 587-589
packages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip6/regression/series.json (1)
3-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the explicit model-series discriminator to these fixtures.
These model trace entries still only carry
source_idandreference_source_id; they never emitkind: "model", even though the same file now emitskind: "reference"for reference traces. That leaves the regenerated bundle out of sync with the new series contract, and the matching cmip7 fixture has the same gap.Also applies to: 820-826
🧹 Nitpick comments (1)
packages/climate-ref-ilamb/tests/unit/test_standard_helpers.py (1)
129-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease lock
index_unitsinto this happy-path test as well.This helper now owns the typed presentation fields, but the series test never asserts
index_units. Adding that check would pin the new contract down and stop this path regressing quietly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aaf57e5-53f9-4237-b42c-049dac12e324
📒 Files selected for processing (83)
changelog/777.feature.mdpackages/climate-ref-ilamb/src/climate_ref_ilamb/standard.pypackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/emp-gleamgpcp2.3/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/emp-gleamgpcp2.3/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/emp-gleamgpcp2.3/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/emp-gleamgpcp2.3/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-fluxnet2015/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-fluxnet2015/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-fluxnet2015/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-fluxnet2015/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-wecann/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-wecann/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-wecann/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/gpp-wecann/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/lai-avh15c1/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/mrsos-wangmao/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/mrsos-wangmao/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/mrsos-wangmao/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/mrsos-wangmao/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip6/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip6/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip6/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip6/regression/series.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip7/manifest.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip7/regression/diagnostic.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip7/regression/output.jsonpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip7/regression/series.jsonpackages/climate-ref-ilamb/tests/unit/test_standard_helpers.py
…ce_id Reference plots carried a source_id="Reference" sentinel; emit kind="reference" and reference_source_id instead so the output bundle matches the series/scalar contract. Also lock index_units into the series unit tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Brings ILAMB series and scalar metric values onto the shared metric-value contract (
kind, clean presentation units, and reference identity on scalars) introduced in #772.Previously ILAMB flagged references with a
source_id == "Reference"sentinel, emitted raw pint-repr units (e.g."gram / day / meter ** 2"), and omittedreference_source_idfrom scalars, so comparison stats could not be grouped by which reference they scored against.This PR:
kindon every series: reference traces (Reference.nc) getkind="reference"and drop the"Reference"source_idsentinel, while model traces getkind="model"and keep bothsource_idandreference_source_idso the two group together."kg / meter ** 2 / second"and"kg m-2 s-1"both become"kg m-2 s-1"), and populates the typed presentation fields (value_units,value_long_name,index_units,calendar). This also fixes a latent bug wherecalendarwas read after the index was converted to ISO strings and so was never captured.reference_source_idandkindto the scalarjson_structure(placed outermost so the raggedstatisticlevel stays terminal).test_case_versionbumps change;make regression-gatereplays all 20 manifest-backed cases with zero drift.Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit
New Features
kindandreference_source_id.calendarwhere available).Bug Fixes
Tests