Skip to content

Show simulation results for arrayed and unit-error'd variables#611

Merged
bpowers merged 3 commits into
mainfrom
core-arrayed-sparkline-canonicalize
May 21, 2026
Merged

Show simulation results for arrayed and unit-error'd variables#611
bpowers merged 3 commits into
mainfrom
core-arrayed-sparkline-canonicalize

Conversation

@bpowers
Copy link
Copy Markdown
Owner

@bpowers bpowers commented May 21, 2026

Summary

Arrayed variables — and any variable carrying a unit error — showed no simulation results in the editor, even though the engine simulates them correctly. This PR makes results display for scalar, 1-D arrayed, and multi-dimensional arrayed variables, on both the canvas sparklines and the variable-details chart, including when a variable has non-fatal unit errors.

Surfaced on C-LEARN, whose substantive variables are arrayed over a scenario dimension and which carries 481 unit errors. The engine is fine throughout: isSimulatable() is true and the run produces 3607 valid series; the results just weren't reaching the diagram.

Root causes

  1. Arrayed data never attached. projectAttachData (src/core/datamodel.ts) rebuilt per-element lookup keys from a Dimension's subscripts, which preserve the model's original-case names (Deterministic, High_2xCO2_sensitivity), while the simulation keys series by canonicalized names (high_2xco2_sensitivity) → 0 matches → empty data. Multi-dimensional arrayed variables were skipped outright by a dimNames.length !== 1 guard.
  2. Unit errors hid the chart. The variable-details panel (VariableDetails.tsx) replaced the chart with an error list whenever the variable had any error, including a non-fatal unit error.

Changes

  • core: rewrite projectAttachData to group every result series by its base variable ident (text before the first [) and attach all of them. Scalar (1 series), 1-D, and multi-D are handled uniformly; it matches whatever keys the run emitted, so the canonicalization mismatch cannot recur and multi-dimensional variables are included ("plot all the series").
  • diagram: unit errors no longer hide the variable-details chart. Only equation/compile errors (which leave the variable with no valid data) replace it; unit errors are shown as warnings beneath the chart. The chart-vs-errors decision is a small pure helper (variableDetailsView), unit-tested directly.

The display layer already plots every series — LineChart (variable details) draws one line per element and the canvas Sparkline draws one path per element — so attaching the full set is all that was needed.

Verification

  • New unit tests: projectAttachData (original-case 1-D, already-canonical 1-D, multi-D, no-data) and variableDetailsView (no errors / unit-only / equation / both).
  • End-to-end on C-LEARN: variables with attached data went 492 → 873; scalar 100_percent → 1 series, 1-D adjusted_temperature_change_from_preindustrial → 3, multi-D co2eq_emissions_from_hfc → all 63.
  • Full pre-commit battery passes (lint, Rust fmt/clippy/test, TypeScript build+tsc+test, Python).

Ruled out during investigation

LTM (the diagram runs with analyzeLtm=false), unit errors gating simulation (they don't — isSimulatable() is true), and the protobuf round-trip (clean). The orange box the user saw is just the 481 unit-error warnings.

Simulation results key each arrayed variable's per-element series by the engine's canonicalized element name (e.g. `temperature[high_2xco2_sensitivity]`), but a Dimension preserves the model's original-case subscript names (`High_2xCO2_sensitivity`). projectAttachData built its lookup keys from the original-case subscripts, so for any arrayed variable whose dimension elements are not already lowercase the keys never matched and the variable received an empty data array -- the diagram then rendered no sparkline for it.

This silently blanked every arrayed variable in C-LEARN (its scenario dimension is {Deterministic, Low_2xCO2_sensitivity, High_2xCO2_sensitivity}), which is why the model appeared to produce no results even though the engine simulates it correctly and isSimulatable() is true. Canonicalizing the element when building the key restores per-element data (verified end-to-end: variables with attached data went from 492 to 814 on C-LEARN). Multi-dimensional arrayed variables remain unhandled by the dimNames.length !== 1 guard -- a separate limitation.
@claude
Copy link
Copy Markdown

claude Bot commented May 21, 2026

Review — core: canonicalize arrayed element keys in projectAttachData

I traced the fix end-to-end and it is correct:

  • The engine keys per-element series by canonicalized element names: db.rs:5787 builds format!("{}[{}]", ident_canonical, subscript) where the subscript comes from dimension elements that are stored as CanonicalElementName::from_raw(e) (dimensions.rs:159-161).
  • A Dimension.subscripts preserves the model's original-case names (datamodel.ts:1377, populated from json.elements).
  • In production the data map keys come straight from the engine's varNames (Editor.tsx:504-508), so they are canonical.

Canonicalizing each element when building the lookup key (datamodel.ts:1490) therefore correctly aligns the constructed key with the engine's series key. The already-canonical case is unaffected (canonicalize('boston') === 'boston'), so there's no regression — confirmed by the second test. The first test genuinely exercises the bug (original-case subscripts would yield an empty array pre-fix). canonicalize was already imported, and the TS/Rust canonicalization agree for these element names.

No blocking issues found. The dimNames.length !== 1 multi-dimensional limitation is correctly called out as pre-existing and out of scope.

Overall correctness: correct.

Generalizes projectAttachData to group every result series by its base variable ident (the text before the first `[`) and attach all of them. A scalar gets its single bare-keyed series; an arrayed variable gets every per-element series the simulation emitted, for any dimensionality. This supersedes the previous 1-D-only path (which also had to canonicalize element names to match keys) and removes the `dimNames.length !== 1` guard that silently dropped all multi-dimensional arrayed variables.

Because grouping matches whatever keys the run produced rather than reconstructing them from a Dimension's original-case subscripts, the element-name canonicalization mismatch can no longer recur. The display layer already plots every series (LineChart in VariableDetails draws one line per element; the canvas Sparkline draws one path per element), so attaching the full set is all that was needed. Verified end-to-end on C-LEARN: variables with attached data went 814 -> 873; a scalar attaches 1 series, a 1-D arrayed var 3, and the multi-dimensional co2eq_emissions_from_hfc all 63.
@claude
Copy link
Copy Markdown

claude Bot commented May 21, 2026

Review

The core change in projectAttachData is functionally sound: grouping every result series by the ident before the first [ correctly attaches scalar, 1-D, and multi-D arrayed series, and matches the engine's already-canonicalized keys directly — which eliminates the original-case subscript mismatch the first commit was patching. canonicalize, variableIsArrayed, and variableEquation remain used elsewhere in datamodel.ts, so no dead imports were introduced. One non-blocking finding below.

[P3] Stale test comments describe the abandoned key-reconstruction approach

src/core/tests/datamodel.test.ts:1320 — The comment // ordered by the dimension's declared subscript order no longer matches the shipped implementation. The final projectAttachData groups series in the iteration order of the input data map (which in production is run.varNames, Editor.tsx:505); it does not sort by dimension subscript order. The expected [[1,2,3],[4,5,6],[7,8,9]] holds only because this test happens to build its data Map in that order — it is a property of the input, not a dimension-order guarantee, and would silently change if the input order changed. Relatedly, the block comment at lines 1300–1304 states that projectAttachData "must canonicalize each element when building the lookup key," which describes the first commit's logic; the shipped code does no key reconstruction or canonicalization, so a reader will look for logic that is not there. Per CLAUDE.md ("update nearby comments in the same commit so docs and code stay aligned"), these are worth correcting.

    // ordered to match the iteration order of the input data map

Overall correctness: correct

The patch is free of functional bugs; existing behavior and tests are preserved, and the generalization to multi-dimensional arrayed variables is correct. The only issue is the stale (non-blocking) test comments noted above.

The variable details panel previously replaced the chart with an error list whenever the variable had ANY error, including a non-fatal unit error. A unit error does not stop the variable from simulating, so it has valid data worth charting; hiding the chart meant a model with many unit errors (e.g. C-LEARN, 481) showed almost no results in the details panel.

Now only genuine equation/compile errors (which leave the variable with no data) replace the chart; unit errors are surfaced as warnings beneath the chart instead. The chart-vs-errors decision is a small pure helper (variableDetailsView) so it is unit-tested directly. Together with the projectAttachData fix, both scalar (one line) and arrayed (one line per element) variables now display their results in the details chart even when they carry unit warnings.
@bpowers bpowers changed the title core: canonicalize arrayed element keys in projectAttachData Show simulation results for arrayed and unit-error'd variables May 21, 2026
@claude
Copy link
Copy Markdown

claude Bot commented May 21, 2026

Code review

I reviewed the rewrite of projectAttachData (grouping result series by base ident), the VariableDetails chart/error refactor, and the new tests. The core logic is sound: grouping by the substring before the first [ correctly attaches scalar (bare key), 1-D, and multi-dimensional arrayed series, sidesteps the original-case vs. canonical key mismatch entirely, and the empty-series guard plus the variableDetailsView helper behave as their tests describe. The attached series order is now the engine's varNames order rather than the dimension's declared order, but that's purely cosmetic — LineChart labels from series.name, the canvas Sparkline colors by index, and no code maps data[i] back to a subscript — so it isn't a correctness regression.

[P3] Misleading test comment about series ordering

src/core/tests/datamodel.test.ts:1320

The comment states the attached series are "ordered by the dimension's declared subscript order," but projectAttachData no longer consults the dimension; it preserves the data Map's iteration order. The assertion passes only because this test inserts the data entries in dimension order. The comment documents an ordering guarantee the code doesn't actually provide and could mislead a future maintainer — consider rewording it to say the order follows the input series order.

Overall correctness: correct

The patch is free of blocking issues. Existing behavior is preserved (unit errors still render alongside equation errors; scalars still attach their single series) and the new multi-dimensional path is exercised by tests. The single note above is a non-blocking documentation nit.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.86%. Comparing base (d15ea7e) to head (f860bfa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #611   +/-   ##
=======================================
  Coverage   82.85%   82.86%           
=======================================
  Files         261      261           
  Lines       69836    69836           
=======================================
+ Hits        57866    57867    +1     
+ Misses      11970    11969    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bpowers bpowers merged commit a8cd392 into main May 21, 2026
15 checks passed
@bpowers bpowers deleted the core-arrayed-sparkline-canonicalize branch May 21, 2026 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant