Preparing reactivity tests + fixes for error bands - #138
Open
olivhoenen wants to merge 10 commits into
Open
olivhoenen wants to merge 10 commits into
olivhoenen wants to merge 10 commits into
Conversation
Measuring first, so every later reactivity change has a before/after number instead of an assertion. The benchmark builds the canvas from issue iterorganization#121: a 2D equilibrium/time_slice/profiles_2d/psi heatmap plus a 1D grid with two traces. It asserts on counts - backend requests and Plotly redraws - and never on wall-clock times, which are noise on a shared runner; timings are still recorded and printed. Renderer-side counters live in utils/perf.ts and are installed only when E2E_TEST=true, matching the convention already used to disable Mantine transitions. Plotly redraws are counted through react-plotly's public onAfterPlot event, so nothing depends on the bundled plotly instance. The spec sits in src/tests/perf/, which keeps it out of `npm run test:e2e` (whose glob is not recursive) and behind the new `npm run test:perf`. Three of its four guards fail today, which is the point of committing them: toggling one panel's edit flag redraws the untouched panel 6 times, two slider steps cost 12 redraws (3 on an unrelated panel), and reopening a metadata tab re-downloads 2 plot_data payloads. BASELINE.md records the numbers. The disruption dataset is used rather than the scenario one because it carries several time slices, so the coordinate sliders are actually operable. Assisted-by: Claude/opus-5
A data entry does not change while IBEX is open, so a response stays valid for the whole session. fetchFromApi is the single place every renderer->backend GET goes through, so the cache sits there. The cache stores the response body TEXT and each caller parses its own copy. That is a correctness requirement, not an optimisation: fetchDataPlot post-processes the parsed response in place (it renames data.name, rewrites coord.target, tensorizes irregular data, runs the non-idempotent transformComplexData, then replaceNullsWithNaN) and five call sites then alias the result straight into the store with `plot.yData = response.data.value`, where transposeAxis and applyRange go on to mutate it in place. Sharing one parsed object would alias one grid's data to another's and corrupt the cached entry with it. Also: in-flight de-duplication so concurrent identical requests share one round trip, an LRU bounded by total and per-entry bytes so a large 2D payload cannot evict the session (it is de-duplicated but not retained), and getConfig() resolved once instead of crossing the Electron IPC boundary on every request. /info/version opts out - the header polls it as a liveness probe. Measured on the benchmark canvas: reopening a metadata tab drops from 6 requests to 1 and no longer re-downloads plot_data at all, which is the third benchmark guard now passing. First open drops from 4 requests to 2. resetAppState() clears the cache between specs. Without it an earlier spec warms the cache and a later one exercises a different path - which is exactly how two data-manipulation specs failed while passing in isolation. Assisted-by: Claude/opus-5
react-plotly.js decides whether to redraw by comparing data, layout and config by reference, so an object literal passed inline forces a full Plotly.react on every render however little changed. Both plot components did exactly that, and the heatmap also rebuilt its trace array with a full copy of the z matrix each time. - config moves to two shared frozen constants (plotConfig.ts); it only ever varied by staticPlot, which has two values. - the heatmap trace array becomes a useMemo, and x/y/z are copied once where they are computed - an effect that only runs when the data or coordinates change - instead of on every render. Plotly keeps and mutates what it is given and those vectors index into the store, so exactly one copy is kept. - isMatrixPlottable built a tf.tensor over the whole array, never disposed, purely to read whether the last dimension was non-zero, and ran in the JSX body of both plot components several times per render. A plain scan computes the same thing allocating nothing, keeping the old semantics including the cases where tf.tensor used to throw (ragged, mixed depth, mixed scalar types, complex pairs). Heatmap2D no longer imports tensorflow at all. - getVectorData deep-cloned every coordinate, including its full data array, only to sort by axeIndex and read valueIndex. It now projects onto those two numbers first. This runs on every slider tick and on every plot's render path. Also fixes two error-reporting bugs that in-flight de-duplication exposed: the HTTP status now travels on the error object rather than in a closure the joining caller never runs - without it an expected 464 on an error-band node lost its status and was reported to the user as "Unable to contact the server" - and handleError no longer notifies twice for one shared rejection. resetAppState dismisses leftover notifications: they expire on a timer, so the suite was relying on being slow enough for that to happen between tests. Measured: toggling a UI flag drops from 11 redraws to 2, two slider steps from 12 to 6, reopening the metadata panel from 19 redraws and 6 requests to 9 and 0. The slider no longer redraws unrelated panels at all, so that guard now passes. Assisted-by: Claude/opus-5
The plot components and the grid panel only ever wrote to the store, yet each subscribed to all of it, so any change anywhere re-rendered every panel and handed Plotly new props. - SimplePlotly and Heatmap2D no longer subscribe at all: both read useIbexStore.getState() inside the handlers and effects that write, which is the pattern handleDeleteGrid already used. This also removes a structuredClone of every plot's data that ran just to change a title. - GridLayoutPlot subscribes only to whether any URI is selected - the one value it reads while rendering - and reads the rest at call time. Its three panel handlers lose their [active] dependency and become stable for the component's lifetime. - GridLayoutPlot is memoized. That only works because handleEditGrid now keeps the identity of grids it is not changing, instead of rebuilding every grid object to set two booleans that were already false. Measured: toggling a UI flag drops from 42 component renders to 14, and two slider steps from 40 to 28. The untouched panel still redraws once per toggle, down from 6. Closing that needs the data/config store split: the configuration is still replaced whole on every write, so VisualizationPlot re-renders and react-grid-layout clones every child on the way through. Assisted-by: Claude/opus-5
Entering edit mode sets `static` on one grid, which changes the layout react-grid-layout derives from its children, so RGL reports onLayoutChange. handleUpdateLayout then rebuilt every grid object from that report - including the ones that had not moved - which handed the memoized panels new props and redrew the untouched heatmap. - Grids whose x/y/w/h/static match the report keep their identity. - A report that changes nothing writes nothing, so a layout event no longer flags the configuration as unsaved on its own. - minH/minW are derived the same way the data-grid prop derives them, instead of being hard-coded to a value that only suited grids with coordinates. - handleUpdateLayout reads the store at call time and loses its [active] dependency, the pattern the panel handlers already use. This closes the last benchmark guard: toggling one panel's edit flag now redraws only that panel (the untouched heatmap goes 6 -> 0 against the original baseline) and costs 4 component renders instead of 42. All four scenarios in `npm run test:perf` pass, and the e2e suite is green. The container deliberately subscribes to `active` rather than `active.dataPlot`: handleNewPlot pushes a new grid into that array in place, so its identity does not change when a panel is added. Assisted-by: Claude/opus-5
node_info passed `show_error_bars` positionally into get_node_info, whose second parameter is `recursive`. With the "see error bars" preference on, every call therefore walked and serialized the metadata of the entire subtree - which NodeInfoResponse then discarded, since NodeInfoChildModel has no `children` field - and the error bar filter never ran at all. The filter being skipped had no visible effect (the recursive branch returns every child, which is what the flag asks for), so the cost was the only symptom: on the disruption fixture, node_info on summary:0 takes 0.20 s with the flag on against 0.13 s with it off, while on equilibrium:0 the difference is lost in the filled-path scan that dominates there. - Pass the flag by keyword. - Forward it through _jsonify_metadata's recursive branch, which dropped it. - Guard the delegation in a test: the response looks identical either way, so assert on the arguments the endpoint hands the service. Assisted-by: Claude/opus-5
react-plotly.js compares `layout` by reference, so each of the fifteen effects
that called setLayoutPlot handed it a new identity and cost a redraw as a panel
appeared: seven in SimplePlotly, five in usePlotLayout, six in Heatmap2D.
Each component now derives its whole layout in one useMemo:
- usePlotLayout returns a memoized {xaxis, yaxis, yaxis2} fragment (grid display
and axis types) instead of taking a setter. Its rule that string x data forces
a category axis, and that a category axis returns to linear once the data is
numeric, is unchanged - but it no longer assigns to itemDataGrid.xAxisData in
place. The configured type is persisted and read back by the customization
panel, so it is written through updatedConfiguration, guarded so it writes
only on the two transitions the effect handled.
- SimplePlotly derives title, height, width, the axis titles and the whole
y2 block. Its `dataEntries` state existed only to gate those effects and is
gone; the titles derive from itemDataGrid.plot directly.
- Heatmap2D derives the same, plus the 1:1 ratio (which no longer needs a state
mirror of forceXyRatio nor a structuredClone of the layout to patch two
fields) and the category y axis that init3DAxis used to push in.
- What the user does with the mode bar cannot be derived, so it stays in state
and is merged last: rebuilding the layout never discards a zoom or pan.
Also fixes a stale width: the effect depended on [width] but read
layoutPlotWidth, which also depends on how many coordinate sliders are shown.
Redraws when a panel appears: 9 -> 6 on a metadata revisit, 3 -> 2 on first
open, i.e. 19 -> 6 against the original baseline. The four benchmark guards and
the 20 e2e specs pass, and the rendered layouts were checked against Plotly's
_fullLayout (axis titles, types, grid, forced ratio, y2 side).
Assisted-by: Claude/opus-5
Two kinds of waste, both on the paths that run per interaction rather than per fetch. Round trips that were taken one at a time: - The geometry overlays fetched r and z (outline), r/z/width/height (rectangle) and r/z/length_alpha/length_beta/alpha/beta (oblique) one after the other, although the nodes are independent: up to six sequential round trips per overlay, now one Promise.all. - Error bands fetched the upper band, waited, then fetched the lower one; same fix, in both the interpolated and the plain branch. - The interpolation <Select> stayed enabled while a method was in flight, so changing it again started a second grid-wide refetch and the two results raced to write the grid. It is now disabled while loading, and a response whose request has been superseded is dropped. Copies made to change a field: - Toggling "error bands" and switching plot type cloned the whole configuration - every fetched array - to set one field on one grid. They now copy that grid only and keep the identity of the others, so the memoized panels are not re-rendered either. Closing the customization panel likewise. - SimplePlotly cloned itemDataGrid.plot and itemDataGrid.coordinates on every change of either, i.e. on every slider tick. getErrorsAreaToPlot only writes connectgaps, customdata and hovertemplate onto each plot, and reads the coordinates, so the copy is now the plot objects plus the two vectors handed to Plotly; yData and the coordinates are shared. - Deciding whether two grids share a coordinate compared JSON.stringify(a) === JSON.stringify(b), serializing both coordinate arrays of every grid on every slider tick. isSameAxisData walks them with an early exit instead, keeping the old semantics (NaN equals NaN, null equals undefined) because JSON.stringify wrote both as null. The benchmark counts are unchanged - these are cost-per-operation, not redraw-count, fixes - and the fixtures are too small for the timings to show it. The e2e suite passes, but note it does not cover error bands: the spec that did needs a WEST pulse file and is commented out upstream, and no node in the local fixtures carries error bar data. Assisted-by: Claude/opus-5
The committed benchmark runs on the e2e fixtures, which are small enough that the timings say little. The same scenarios were run by hand against a production ITER entry (134173/106: an equilibrium with 720 time slices, plotting profiles_2d/psi as a heatmap - the case in issue iterorganization#121) and compared with commit 5614230, the benchmark before any fix. Plotting the heatmap: 8 -> 4 redraws, 36 -> 26 renders, 6.4 s -> 4.7 s. Stepping a coordinate slider: 6 -> 4 redraws per step. Toggling a UI flag: 3 -> 1 redraw. No change in the number of backend requests, which was already minimal for this canvas. Also records why the run has to be driven from the DOM rather than through getTestState, and that what is left of the plotting cost is the payload: the backend does not downsample 2-D nodes. Assisted-by: Claude/opus-5
Three defects on the error-band path, none of them covered by a test: the CI fixtures carry no error data, so this was checked by hand against the WEST entry 58463/0 and a fixture written for the symmetric case. A band trace never copied its main trace's `yaxis`, so a band belonging to a y2 trace was drawn against y1 - Plotly's default - and dragged y1's autorange onto y2's scale. On a temperature/density panel, switching bands on flattened temperature (~1e2 eV) against the axis floor of a density band (~1e19 m^-3). The two band nodes were fetched together and neither was formatted until both had arrived, so one missing node threw away the other. A quantity that stores only `_error_upper` - the IMAS convention for a symmetric error - therefore got no band at all, although `getErrorsAreaToPlot` has always known how to mirror a single band around its trace. Each node is now fetched on its own and whichever exists is drawn; only when both are absent is that reported, as a warning naming the node, since it is the expected case for most quantities. The warn and error branches were also the wrong way round. Finally `userRelayout`, which carries what the user did with the mode bar across layout rebuilds, accumulated the raw `plotly_relayout` payload and was never pruned: the ranges of an old zoom outlived the autoscale meant to clear them, and both outlived the panel being pointed at other data. It is folded per axis now, `range` and `autorange` replacing one another, and merged inside each axis rather than at the top level, which is what makes a saved zoom actually restore. It resets when the plotted nodes change, with indices normalised away so that stepping a coordinate slider keeps the zoom. Assisted-by: Claude/opus-5
olivhoenen
requested review from
SimonPinches and
prasad-sawantdesai
as code owners
September 21, 2026 11:44
Documentation build overview
134 files changed ·
|
Contributor
Author
|
Some level of manual tests in case you have working state files with v0.3 would be appreciated @prasad-sawantdesai @paulotex |
|
I could not find my IDS with error bands, sorry. But I confirm that no new errors have been introduced, my saved workspaces worked fine. |
paulotex
approved these changes
Sep 22, 2026
paulotex
left a comment
paulotex
left a comment
There was a problem hiding this comment.
LGTM, tested with saved workspaces with no issues
This branch has not been deployed
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
2 participants
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.
This PR tackles several things: