feat(fit): the Fit wizard — build a model, fit the scan, commit component maps - #86
Open
CSSFrancis wants to merge 7 commits into
Open
feat(fit): the Fit wizard — build a model, fit the scan, commit component maps#86CSSFrancis wants to merge 7 commits into
CSSFrancis wants to merge 7 commits into
Conversation
…55, #56, #58) The first UI on top of spyde.fitting. Two tabs, matching the Orientation caret's shape: Model builds the model component by component, Run fits it and commits the maps. Layout is horizontal by preference — a component's parameters sit side by side so the caret grows wider rather than taller. #56, the picker: components are added from a "+ Component" POPUP that shows each one's SHAPE, not just its name. The backend samples every offerable component at defaults over the CURRENT signal axis and sends a normalised polyline the renderer draws as a sparkline. Seeding matters — a default Gaussian sits at 0 with sigma 1, which is off-screen on a 200-800 eV axis, so every peak shape would otherwise preview as an identical flat line. #58, the maps: Commit ships one integrated-area map per component through commit_result_tree, which already gives the strain-style toggle (click one, cmd-click to tile). No new display code. The preview is ONE spectrum, not the grid: a model edit has to feel instant, and fitting 65k positions per keystroke would not. fit_run is the only thing that touches the whole scan. The caret rebuilds itself from `fit_state` and never edits a local copy — the backend owns the model, and a caret that mutated its own would drift out of step the moment an edit was rejected, then show a model that is not the one being fitted. TWO REAL BUGS that only running the app could find; both handler-test suites pass either way: 1. A click on a caret control was SILENTLY LOST. Capture-phase listeners showed `mousedown -> the button's span, mouseup -> mdi-area`: the mousedown bubbled to the window, re-rendered the subwindow subtree, and replaced the control between down and up, so React never saw a click. The add simply did not happen, with no error anywhere. Fixed by keeping mousedown inside the caret. (dispatchEvent worked throughout, which is exactly what makes this look like a test problem rather than a UI one.) 2. `emit` was imported as `from ipc import emit`, which binds the original at import time — the test fixture patches `ipc.emit`, so every message assertion would have silently observed nothing. Imported as a module now. Backend: 26 handler tests (including the StrictMode open/close/open contract). UI: fit_wizard.spec.ts drives the real app end to end — toolbar button, palette sparklines, model building, a parameter edit echoed back from the backend, the fit, teardown/reopen, and Commit opening a window with a chip per component. Screenshots at every stage.
#57 — every positioned component gets on-plot handles: a POINT handle driving its centre and height, and a RANGE band driving its width. Dragging writes straight back into the model and redraws, and the numeric fields refresh from the same state, so both directions agree. Ported from anyplotlib's interactive-fitting example. The conversions are the substance. For most components the fitted amplitude is an AREA, not a height, so storing a dragged y straight into `A` would jump the curve by sigma*sqrt(2*pi) the instant it was touched. A width drag likewise holds the peak HEIGHT fixed while changing sigma — otherwise the curve moves away under the cursor, which reads as it fighting the drag. PERSISTENCE: the model and the fit now live on the TREE (`tree.fit_spec`, `tree.fit_result`), not on the caret controller. They are results, and the ownership map in actions/README.md §3 puts results on the tree. A model costs real effort to build and a fit costs minutes; closing the caret to get it out of the way must not discard either. tree.close() still disposes both. THREE MORE BUGS THE RUNNING APP FOUND, all invisible to the handler tests: 1. The live preview NEVER DREW. anyplotlib's add_line takes `label`, not `name`, and the TypeError was swallowed by this method's own except. The previous spec passed end to end while the plot showed nothing — I only caught it by looking at the screenshot. 2. Preview lines ACCUMULATED: remove_line takes an id or a Line1D handle, not a label, so every redraw stacked another line and the legend filled with `fit_preview` entries. The handle is kept now. 3. A new component arrived with amplitude 1 against counts of 1e5 — the curve was a flat line on the axis, the handles sat at y=0, and the fit started five orders of magnitude from its answer. All three read as "the model does nothing". scale_to_data() now sets the linear amplitude from the spectrum itself, generically: evaluate at unit amplitude, scale the peak to a fraction of the data's range. 39 backend tests; the e2e now also asserts the model survives close/reopen, and its screenshot is where the preview curve and the handles are actually checked — nothing else in the suite can see them.
Owner
Author
#57 drag handles + a persistent model#57 — every positioned component now gets on-plot handles: a point handle driving its centre and height, and a range band driving its width. Dragging writes straight back into the model and redraws; the numeric fields refresh from the same state, so both directions agree. Ported from anyplotlib's interactive-fitting example. The conversions are the substance. For most components the fitted amplitude is an area, not a height — storing a dragged Persistence — as you asked. The model and the fit now live on the tree ( Three more bugs the running app foundAll invisible to the handler tests, and the first is the one worth dwelling on:
Verified39 backend tests, and the e2e screenshot now shows what it should: one |
…background Three things, two of them from using it. "FIT SPECTRUM" (requested): fits only the spectrum on screen and writes the result back into the model, so the next nudge starts from a fitted position. Building a model is a loop — place a component, look, adjust — and fitting the whole scan to check one guess is the wrong unit of work: it costs seconds to minutes and answers a question about one pixel. Same engine, same spec, one row of data. It deliberately does NOT set a scan result: one spectrum is not a map, and offering Commit after it would export a component map built from a single pixel repeated everywhere. The other button is now "Run scan", so the two read as what they are. HANDLES MOVE IN PLACE instead of being rebuilt (the "gaussians don't move" report). fit_set_param used to call sync_widgets(), which tears down and recreates every widget — on every keystroke, several times a second. That makes the handles flicker and can pull one out from under the cursor mid-reach. update_widgets() now moves them via the widget's own set(), and rebuilding is reserved for a changed component LIST. A drag skips the handle being dragged, because writing a position back to the widget under the user's finger fights the drag. I could not reproduce a parameter edit failing to move the CURVE — in the app, typing a new centre moves both curve and handles — so the churn above is my best explanation for what was seen. If it persists, which parameter and how many components would narrow it. #65: fit_from_composition — type the elements, get a populated model, then drag the lines. EELS models are tabulated on the way in (#63), or the model is correct but falls back to hyperspy's per-pixel fitting, which is the difference between seconds and minutes on a real scan. #64: background_action — drag a window over a background-only region; the model is fitted to THAT SPAN ONLY and extrapolated across the whole axis, which is the entire point of a background model (it says what the background would have been under the peaks, where it cannot be measured). Fitted per navigation position, because a spectrum image has a different background per pixel and subtracting one pixel's fit everywhere would imprint a thickness map on the result. Seeded from the data INSIDE the window: a pre-edge window sits far below the peak, so scaling to the global max starts it an order of magnitude high. 46 backend tests; the e2e passes end to end.
…erly The drag did nothing to the model, and the reason is embarrassing: anyplotlib hands the dragged widget back on `event.source`, so the position is `event.source.x`. I read `event.x`, which is None — so every drag silently updated nothing while the handle still moved on screen, because the widget draws itself. That is exactly why it looked like "the curves don't move". Rewritten to follow the interactive-fitting example rather than approximate it: - Lines are updated with Line1D.set_data IN PLACE. Removing and re-adding a line per drag frame is heavy and does not repaint mid-drag. - ONE LINE PER COMPONENT plus a dashed sum, instead of a single summed curve. With several overlapping peaks, a sum tells you the total is wrong but not which component to grab. - The `_syncing` guard, because moving the handles in response to a drag re-enters the handler and the widget and model chase each other. - The range widget uses style="fwhm" at y=height/2, and the point widget drops its crosshair — the example's geometry, which reads as a peak handle and a width band rather than two unrelated markers. Verified with a REAL pointer drag in the app, not a synthesised event: grabbing the handle and dragging left moves centre 500 -> 296 eV, and the curve, both handles and the caret's fields all follow. A screenshot confirms it. "Fit spectrum" moved to the Model tab (requested). That is where the loop is — add, look, nudge, fit this one, look again. Run scan stays on its own tab as the separate expensive thing you do once the model is right. The drag test now pins the `event.source` contract directly, so this specific mistake cannot come back silently.
…d spectrum Reported as "Fit spectrum doesn't work at all", and the way it failed is worse than not working: it said CONVERGED and drew a model at about half the height of the data. current_spectrum() reconstructed the on-screen spectrum from signal.data plus a navigator index. The index was not where it expected, so it fell through to the mean over navigation — and a fit against the mean converges perfectly well. The status was true; the answer was about a spectrum nobody was looking at. plot.current_data is the authority: it IS the array the plot is displaying, already resolved through whatever navigator, region-integration or derived-view path produced it. Read that. The navigation mean survives only as a stand-in for the PREVIEW before the first frame lands, and it logs when it is used. Why nothing caught this: the handler tests build their own signal and never have a navigator, so the fallback WAS the displayed spectrum for them. It needed the real app, a real navigator position, and an assertion about the fitted VALUES rather than about a status string. fit_two_gaussians.spec.ts is that test, on the dataset this is meant for (tutorial_spectroscopy = hyperspy's two_gaussians). It asserts the model's peak height, computed from the fitted parameters — a HyperSpy Gaussian's A is an AREA, so the height is A/(sigma*sqrt(2*pi)) summed over components. Fitting the navigation mean gave ~460 against data peaking at ~1050; the fix gives ~1050. The threshold separates those two outcomes without pinning an exact fitted value, and a second assertion stops a degenerate fit that zeroes one component from passing on the other's back. Four backend tests pin the same contract directly, including that a painted spectrum always beats the mean and that a stale/odd-shaped current_data (a dask Future mid-transition) is ignored rather than fitted.
… icon PER-POSITION MEMORY. Each navigator position now remembers its own fitted parameters, on the tree beside the model. Scrubbing back to a pixel shows what was found THERE rather than whatever the last pixel left behind. The store is sparse — a dict keyed by position, not a (P, n) array — because it fills in as the user explores, and a full allocation would claim every position had been fitted. It is cleared whenever the component list changes: the vectors are POSITIONAL, so after an add or remove a stored sigma would arrive as an amplitude and nothing would look obviously wrong. ADAPTIVE FITTING (an "Adaptive" toggle). Moving the navigator recalls a stored fit if there is one, and otherwise — with adaptive on — fits the new spectrum seeded from the model as it stands, which is a neighbouring position's answer and therefore a good starting point (the same reason seeded propagation works for a whole scan, #54). Recall wins over refit: same computation, already done. Driven from pointer_UP on the navigator, not pointer_move, or it would queue fits faster than they complete. This also fixes the thing that made the earlier "Fit spectrum" bug possible: current_indices lives on the navigation SELECTOR, not on the Plot. Reading it off the Plot always returned None. There is now a real accessor and a test that exercises it against a REAL session rather than only a fake. SMOOTHER DRAGS. pointer_move and pointer_up are wired to separate handlers because they should do different amounts of work. A move now redraws the curve and nothing else; moving the partner handle and re-sending the whole model both cross the IPC boundary, and doing either at pointer rate is what made the curve lag the cursor. Both happen once, on release. Remove Background gets its OWN icon — it was borrowing Fit's, so the two buttons were indistinguishable. Drawn for 20 px rather than for the artboard: only about three strokes survive at button size, so it spends them on a straight baseline and a down-arrow, a different silhouette from Fit's peak. The first attempt was a dashed background curve — handsome at 150 px, mush at 20, which was the whole complaint. Checked by rendering at 20/28/150 px on the toolbar's own background before committing. 66 backend tests, both e2e specs green.
THE DRAG REGRESSION WAS MINE. Splitting the preview into one line per component went from ONE line update per pointer frame to N+1, and Line1D.set_data is not cheap bookkeeping: it recomputes the axis range and pushes the WHOLE plot state to the renderer. Two gaussians therefore meant three full state pushes per frame where there had been one. That is why it got worse after the last change, and the "smoothness" work in that commit did not touch it. refresh_lines now writes every line's data and pushes ONCE, so it is one push per frame regardless of component count — fewer than before the per-component rewrite, not merely back to parity. It reaches into anyplotlib's line entries because there is no public batched update; the public per-line path stays as the fallback, so an upstream change costs speed rather than correctness. FIT COVERAGE, so the store is visible: the caret shows "N/M fitted" with a filled marker when the CURRENT position has an answer. Without it there is no way to tell a position you skipped from one you fitted, and "why didn't adaptive fill this in" has no answer on screen. WHAT I COULD NOT MEASURE, stated plainly. I tried to quantify drag smoothness through Playwright and produced a number that was mostly harness: a bare page.mouse.move costs ~16 ms here, so 30 synthetic moves take ~600 ms with NO caret open at all, against ~850 ms with one. My earlier "~28 ms per move" was therefore ~70% Playwright. On that bad number I added a redraw coalescer; it showed no improvement, and I have removed it rather than ship an unverified optimisation that also drops frames. The batched push stays because it is a strict reduction in work that is true independent of any timing. Measured for real, on the side: a single-spectrum fit is 33 ms (14 LM iterations). float32 is SLOWER than float64 here (203 vs 109 ms) — worth recording so nobody tries it again. The 109 ms in that comparison was dominated by BUILDING the spec, which goes through sympy in hyperspy's Expression components; reusing a spec is 33 ms.
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.
Closes #55, #56, #58. Part of tracker #48.
The first UI on top of
spyde.fitting. Stacked on #84 (needsModelSpec+ the engine).Shape
Two tabs, matching the Orientation caret: Model builds the model component by component, Run fits it and commits the maps. Layout is horizontal by preference — a component's parameters sit side by side, so the caret grows wider rather than taller.
Components are added from a
+ Componentpopup rather than an always-open palette.#56 — the picker shows shapes, not names
The backend samples every offerable component at defaults over the current signal axis and sends a normalised polyline, drawn as an inline sparkline. "Gaussian" and "Lorentzian" aren't distinguishable by name to someone who hasn't met them.
Seeding matters here: a default Gaussian sits at 0 with σ=1, which is off-screen on a 200–800 eV axis — every peak shape would otherwise preview as an identical flat line.
#58 — component maps
Commit ships one integrated-area map per component through
commit_result_tree, which already gives the strain-style toggle (click one, ⌘-click to tile). No new display code.Two real bugs only running the app could find
Both handler suites pass either way — this is the case CLAUDE.md is about.
A click on a caret control was silently lost. Capture-phase listeners showed
mousedown -> the button's span, mouseup -> mdi-area: the mousedown bubbled to the window, re-rendered the subwindow subtree, and replaced the control between down and up — so React never saw a click. The add simply didn't happen, with no error anywhere. Fixed by keeping mousedown inside the caret.dispatchEventworked throughout, which is precisely what makes this look like a test problem rather than a UI one. It isn't: a user clicking that button gets nothing.emitwas imported asfrom ipc import emit, which binds the original at import time. The test fixture patchesipc.emit, so every message assertion would have silently observed nothing. Imported as a module now.Verification
fit_wizard.spec.tsdrives the real app end to end — toolbar button, palette sparklines, model building, a parameter edit echoed back from the backend, the fit, teardown/reopen, and Commit opening a window with a chip per component. Screenshots at every stage, and I looked at them.Worth your call
Closing the caret disposes the fit. The wizard owns the result, so close/reopen discards it and Commit is no longer offered. Consistent with the other wizards, but a fit is more expensive than most wizard state — happy to persist it on the tree instead if you'd prefer.
Still open in Wave 1's UI: #57 (drag components directly on the plot — the anyplotlib interactive-fitting port).