feat: consume the shared metric-value contract (kind, reference_id, presentation attrs)#32
Merged
Conversation
…ue DTOs SeriesValue now carries kind, reference_id, and normalised presentation attrs (value_units, value_long_name, index_units, calendar), and build_scalar/build_series set kind from the row's dimensions instead of leaving it at its default. Presentation attrs are normalised from the attributes blob via a provider-key fallback map, since ESMValTool and ILAMB use different attribute names for the same concepts. This requires bumping the climate-ref dependency to an unreleased commit on main, since the underlying kind/reference_id/presentation fields don't exist in any published release yet. That bump also raises the Python floor to 3.12 to match climate-ref's own requirement. Note: bumping climate-ref surfaces a schema mismatch between the newer package and the committed sqlite test fixtures (diagnostic.promoted_version is missing from an unrelated versioning migration), breaking the DB-backed integration tests in test_diagnostics.py, test_executions.py, test_datasets.py and test_results.py. This is outside this slice's scope to fix and needs a follow-up before merge.
…_id sentinel calculate_iqr_bounds_by_source_id and detect_outliers_in_scalar_values excluded reference rows from IQR bounds by matching source_id == "Reference". That sentinel breaks down for providers where the reference series carries a real source_id. Both functions now key off the kind dimension (kind == "reference") that sv.dimensions already spreads into the DataFrame, treating a missing/None/ empty kind as a model. If the kind column is absent entirely (older data), fall back to treating all rows as models rather than raising.
The metric-value contract bump pulls in the diagnostic-versioning and kind/reference_id migrations. Run the fixture database through alembic so the committed test data matches the new schema (adds promoted_version and the kind dimension column).
Surfaces kind, reference_id and the presentation attributes (value_units, value_long_name, index_units, calendar) on the generated SeriesValue and ScalarValue types.
…y reference_id Consume the shared metric-value contract's kind and reference_id fields on the frontend instead of inferring role from the source_id === "Reference" sentinel. - seriesChartContent.tsx splits series into model/reference buckets by kind, treating missing or "model" kind as a model series. - createChartData derives isReference from kind instead of array position, and dedups reference series by reference_id, falling back to label-based dedup only when reference_id is absent. - isReferenceItem keys off kind instead of the source_id sentinel. - DimensionedData gains an optional kind field so grouping utilities can access it generically.
…alue Series charts now prefer each series' own value_units/index_units for axis labels, falling back to the card-config units prop when a series doesn't carry them (ILAMB never sets value_units or calendar; ESMValTool does). createChartData also aligns series by their actual index value rather than by array position, so series with different or partially overlapping index arrays no longer get smeared together -- each series contributes its value to the row matching its own index, with gaps left null. calendar metadata is surfaced from series attributes and still relies on ISO-8601 string detection for time-axis selection, since non-standard calendars (360_day, noleap) aren't fully supported by JS Date parsing.
Address review findings on the index-value alignment: - Coerce absent per-series value_units/index_units/calendar to undefined so the app type-checks under the real `tsc -b` build (the nullable client fields were being assigned into non-null return types). - Restore the positional fallback for series that carry no index array, so index-less series render instead of collapsing to an empty chart. - Sort the index-value union ascending so fully interleaved series indices render left-to-right instead of zigzagging. Add regression tests for the index-less and interleaved-index cases.
✅ Deploy Preview for climate-ref canceled.
|
📝 WalkthroughWalkthroughAdds ChangesBackend and frontend kind-based series handling
Sequence Diagram(s)sequenceDiagram
participant SeriesChartContent
participant SeriesVisualization
participant createChartData
participant SeriesCanvas
SeriesChartContent->>SeriesVisualization: regularSeries, referenceSeries
SeriesVisualization->>createChartData: seriesValues, referenceSeriesValues
createChartData->>createChartData: dedupe by reference_id or label
createChartData->>createChartData: align rows by index value
createChartData-->>SeriesVisualization: chartData, valueUnits, indexUnits, calendar
SeriesVisualization->>SeriesCanvas: chartData, effectiveUnits, indexUnits
SeriesCanvas->>SeriesCanvas: render axis labels
Compact metadata
Poem Kind now marks the model and the reference lane, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
backend/src/ref_backend/core/outliers.py (1)
141-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFallback IQR path doesn't respect
kind— reference rows can be misflagged.The kind-aware reference exclusion is only applied in the
source_id-aware branch (Lines 147-155). Whensource_idis absent or the group is too small (Lines 159-165),flag_outliers_iqrruns over all rows includingkind == "reference"ones, and no override forces reference rows back to non-outlier. This means a reference row could be flagged an outlier in this fallback path, contradicting the intended contract that reference values are excluded from outlier logic.🐛 Proposed fix to force reference rows non-outlier in the fallback branch
else: # Fallback to original IQR method if no source_id or insufficient data if len(group_values) >= min_n: iqr_flags = flag_outliers_iqr(group_values.value.to_list(), factor=factor) + if "kind" in group_values.columns: + iqr_flags = [ + False if kind == "reference" else flag + for flag, kind in zip(iqr_flags, group_values["kind"]) + ] else: iqr_flags = [False] * len(group_values) source_id_flags = iqr_flags # type: ignoreNote the new test suite only exercises groups where
source_idis present, so this gap isn't currently covered by tests either.
🧹 Nitpick comments (1)
backend/tests/test_core/test_outliers.py (1)
114-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion in
test_none_or_empty_kind_treated_as_model.The test only asserts
bounds is not None, which would also pass if the None/empty-kind rows were wrongly excluded (as long as ≥min_nother rows remain). Consider asserting bounds equal those computed from the full 4-row set (as done intest_excludes_rows_tagged_kind_reference) to actually confirm inclusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba0fb19a-5a05-41ba-ab79-f0718c8ff985
⛔ Files ignored due to path filters (2)
backend/tests/test-data/tests.integration.test_cmip7_aft/test_solve_cmip7_aft/db/climate_ref.dbis excluded by!**/*.dbbackend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
backend/pyproject.tomlbackend/src/ref_backend/core/outliers.pybackend/src/ref_backend/models.pybackend/tests/test_core/test_metric_values.pybackend/tests/test_core/test_outliers.pychangelog/32.breaking.mdchangelog/32.feature.mdfrontend/src/client/schemas.gen.tsfrontend/src/client/types.gen.tsfrontend/src/components/execution/values/series/seriesCanvas.tsxfrontend/src/components/execution/values/series/seriesVisualization.tsxfrontend/src/components/execution/values/series/utils.test.tsfrontend/src/components/execution/values/series/utils.tsfrontend/src/components/explorer/content/seriesChartContent.tsxfrontend/src/components/explorer/grouping/types.tsfrontend/src/components/explorer/grouping/utils.test.tsfrontend/src/components/explorer/grouping/utils.ts
When a chart mixed indexed and index-less series, the index-less series used the actual index value as an array offset and vanished. Key its positional fallback off the row ordinal instead, so it renders alongside indexed series. Add a regression test for the mixed case.
* origin/main: chore(deps): update actions/checkout action to v7 chore(deps): update astral-sh/setup-uv action to v8.2.0 chore(deps): update dependency pydantic-settings to v2.14.2 [security] chore(deps): update dependency vite to v7.3.5 [security] chore: set Renovate timezone to Australia/Melbourne chore: add changelog fragment for Renovate chore: add Renovate for frontend and backend dependency updates docs(changelog): add fragment for pre-commit tsc -b fix fix(pre-commit): type-check with tsc -b so build errors are caught # Conflicts: # backend/uv.lock
The kind-aware reference exclusion was only applied in the source_id-aware branch. When source_id is absent, the fallback IQR path flagged all rows including kind=="reference" ones, so a reference value could be misflagged an outlier. Override reference rows to non-outlier on that path too, and cover it with a regression test.
Contributor
Author
|
We can't merge this until we have released and rerun. This will required a new minimum climate-ref version |
Replace the raw `rev` git pin with `tag = "v0.16.0"` and set the constraint to >=0.16.0,<0.17. v0.16.0 is the first tagged release that carries the shared metric-value contract (the pinned commit 383bd3f8 sits between v0.15.0 and v0.16.0, so no earlier release contains it). It is not on PyPI yet, so the tag source stays until it lands, at which point the source line can be deleted. v0.16.0 adds the `dataset.version_key` column (alembic head f6a7b8c9d0e1), one revision past the fixture's e5f6a7b8c9d0, so migrate the test-fixture database forward to match. Full backend suite green (202 passed).
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.
Summary
Makes ref-app consume the shared metric-value contract from climate-ref: the first-class
kind(model/reference), thereference_idcontent hash for deduplicating reference series, and the typed presentation attributes (value_units,value_long_name,index_units,calendar). Role, deduplication, axis units and calendar are now driven by these fields instead of inferring roles from thesource_id == "Reference"sentinel, which is removed from production code.What changed
Backend
SeriesValue/ScalarValueDTOs surfacekind,reference_idand the four presentation attributes. The presentation attributes are normalised from the persistedattributesblob with a provider key fallback (value_units←value_units|units,value_long_name←value_long_name|long_name), since ESMValTool and ILAMB use different attribute keys and the typed fields are not persisted as columns.kind == "reference"instead of thesource_idsentinel; missing/Nonekind is treated as a model.Frontend
kind; reference series are deduplicated byreference_id(falling back to label-based dedup only whenreference_idis absent).value_units/index_unitsdrive the axis unit labels, falling back to the existing cardunitsprop;calendaris surfaced for time handling.Testing
uv run pytest src tests→ 200 passed, 1 xfailed;mypyandruffclean.npm run build(tsc -b && vite build) succeeds;vitest→ 219 passed; Biome clean.Follow-ups
tsc --noEmitagainst the solution-style roottsconfig.json("files": []), which type-checks nothing undersrc/. The effective check istsc -b. Worth updating the hook so build-breaking type errors are caught locally.Summary by CodeRabbit
kindmetadata, improving reference deduplication (includingreference_id) and aligning series by matching index values across differing ranges.kind, avoiding misclassification from naming or prior source-id conventions.kindand new optional scalar/series metadata fields.