Fix correctness bugs from the full codebase review#88
Open
matt-pharr wants to merge 16 commits into
Open
Conversation
round(phase/delta_phi) assumed the unwrapped n*delta_phi stayed inside +/-180 deg, but the pair picker chose the widest raw separation (320 deg on fetched DIII-D shots), aliasing every rotating mode to n = 0. Wrap delta_phi to its signed principal value in the core estimators and pick the widest pair that still resolves |n| <= 5 unaliased. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A cos(n*phi - omega*t) mode measured through conj(sig)*ref cross-phases
ramps as +n*phi, but fit_toroidal_mode fitted c - n*phi,
array_mode_spectrogram projected onto an unconjugated template, and
mac_n_spectrum matched e^{-in*phi} shapes - so the phase-fit family
reported -n while the 2-point family reported +n, despite docstrings
claiming they agree. Flip the fit family to +n, fix the service ramp
and poloidal detrend, use the production delta_phi = phi2 - phi1
convention in fixtures, and pin all five estimators to the same signed
n with a cross-family test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ering The remote backend dispatched before the incremental-merge bookkeeping and rsynced the cluster result directly over shot_<n>.h5 - since the cluster file holds only the requested selection, a one-signal custom pull (the GUI's QS custom-signal panel on the default DIII-D remote backend) silently destroyed every previously fetched channel. Dispatch the remote branch after the merge/skip resolution, stage the cluster file beside the local one when a compatible file exists, and fold it in through _write_h5's merge path (time-base dedup, replace-on-conflict, fetched/missing bookkeeping identical to a local incremental fetch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The design matrix and RHS are normalized by the effective sigma (which honors sigma_override and NaN fills), but fit_signal was de-normalized by the dataset's raw signal_sigma - so any GUI edit of the "uncertainty (sigma)" box scaled fit_signal by signal_sigma/override, corrupting residual, chi_sq, and red_chi_sq while the coefficients stayed correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fit_cond is 1/rcond for the lstsq inversion, but its default (10) was the GUI's "warn when K > 10" trust threshold, not OMFIT SLCONTOUR's 1e3 reference cutoff. Any fit with K in (10, 1e3) silently zeroed basis directions the reference fit keeps, and the reported K(eff) stayed <= 10 by construction - the quality panel looked healthiest exactly when the regularization was distorting the fit. Align the core, service, and GUI defaults on 1e3 and leave the trust threshold to contracts.quality_for_k. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two silent-loss bugs on the tree paths: - The KSTAR _tree_transport branch never passed tree_signals to _fetch_mds_tree, so Ip/B_T/kappa were dropped from every KSTAR pull (absent from fetched AND missing lists) and helicity fell back to its default. Fetch them on the same connection after the sensors, first usable candidate wins, window-sliced (not resample()d onto the sensors' microsecond grid) - mirroring the NSTX path. - _resolve_pointnames mapped queried-pointname -> canonical id in a plain dict, so a per-era alias equal to another sensor's pointname (kstar.json: \MC1T10 -> \MC1P03 since shot 17377) collided: both fetched channels were relabeled to one name and the HDF5 writer silently dropped one. Query the node once, let the sensor literally named by the pointname keep it, and report the alias as skipped. KSTAR live behavior needs on-device verification (new plasma-node requests over the VPN); the changes are otherwise covered by offline stub-connection tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A dropped tunnel/ControlMaster mid-pull marked every remaining channel ok=False, and _write_h5 recorded them in channels_missing - so the next same-window pull treated them as already-attempted and silently skipped them, leaving the shot file permanently incomplete without --force. Classify failures at every fetch site (transport vs no_data, by exception type with message-word fallback) and exclude transport failures from the cached missing set; they are retried on the next pull while genuine no-data channels stay cached. Closes #63. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
compute_spectrogram used raw |conj(S2)*S1| with no window-power or fs scaling, yet rms_by_mode applied the same sqrt(sum(power)*df) formula as cross_spectrum (whose csd is a proper density) - so the n-spectrum axis labeled "rms amplitude" was off by a data-dependent factor of thousands. Apply csd's density scaling (1/(fs*sum(win^2)), doubled off DC/Nyquist); a unit sine now reports 0.707 RMS through both estimators. Log-power views only shift by a constant; coherence and phase are ratios/angles and are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four service-seam fixes from the review: - refresh() cleared every lru_cache except _real_theta, so a re-pull with more probes kept serving the stale channel->theta map and the poloidal nodes 422'd until server restart. - The QS amplitude/phase_t 1-sigma bands rode in untyped meta.sigma, which export.py drops - the downloaded HDF5 lacked the band shown on screen. Move them to the contract's per-series lower/upper (already exported, same as the rotating mode-shape nodes) and read those in the GUI. - channel_usage marked kappa/ip/bt and the QS midplane array as "unused / droppable" although the theta* correction, QS helicity, and the QS fit consume them. - NaN/Inf anywhere in a node payload was an opaque 500 (starlette serializes with allow_nan=False). Raw-signal nodes now emit non-finite samples as JSON null (a Plotly gap) via contracts.json_finite; zrange/chi^2/K scalars are guarded; and quality_for_k now matches contract.ts on +/-inf. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fixes Frontend correctness fixes from the review, verified with Playwright against the live app: - useNode's stale-while-revalidate is now param-scoped only: a machine switch drops the stale node instead of rendering the previous shot's data under the new shot's labels (and export descriptors). Adds a retryKey so an identical-key fetch can re-run: the QS Plot button and a new Retry button recover from transient failures (previously impossible without jiggling a parameter). - QS tab: only a 422 is diagnosed as "shot lacks the QS array"; other failures show a retryable error instead of a confidently wrong banner. Per-node failures surface in an error strip and in the section placeholders instead of eternal "loading..."; the mode-map panel shows the server error instead of fake "Computing..." progress. Removed the hardcoded shot-specific 3140 ms cursor write; the Plot button's dirty highlight compares params by value. - PullControl snaps per-device defaults whenever the store's device changes - the left rail's picker previously left backend "remote" (and a DIII-D window) active for tree devices while the select displayed "mdsthin". - SensorsTab resets its per-device set overrides on machine change (they are keyed by set name; carrying them over rendered a permanently empty sensor scene). - Both heavy tabs use selective store subscriptions instead of re-rendering every Plotly panel on each credential-field keystroke. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The array n-map's dedicated STFT was computed over a fixed 0-50 kHz band with the GUI's fmin/fmax applied only as a crop, so raising f_max past 50 kHz showed nothing above it - the rotating view read as hard-capped at 50 kHz (the power spectrogram already extended to the data's Nyquist). Thread the requested fmax into _array_mode_spec as the compute-band ceiling, quantized to 50-kHz steps so crops within a step still hit the cached STFT. Verified on shot 174446: fmax=120 now yields the map out to the 100 kHz Nyquist. Closes #85. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stale docs
- tests/test_nodes.py resolved its shot by taking the first machine,
but list_shots() sorts ids as strings, so the NSTX shot '204718'
sorted before the DIII-D '990000' and the whole module silently ran
against NSTX: the DIII-D family-mixing guard was tautological (every
NSTX name maps to family "?") and the contour test permanently
skipped. Pin the DIII-D fixture shot and drop the try/except->skip
guards - the fixture guarantees the arrays, so a builder crash is
now red. Strengthen the K-finiteness test to actually check K, and
remove test_cli's dead `or True`.
- CI treated pytest exit 5 ("no tests collected") as success - a
collection break would go green with zero tests run. The suite is
400+ tests; drop the tolerance.
- Declare pexpect (kstar_transport imports it lazily; a wheel install
died at live-pull time).
- CLAUDE.md described the retired _slcontour shim and a phantom
core/quasistationary port (#40/#44 are closed); rewrite the QS
architecture/layout/known-gaps to the promoted core/qs_* reality.
Fix the stale run-dev.sh "QS is a stub" header, contract.ts's dead
"magfit" path, and test_contract_shapes' extra-that-never-was note.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fills the highest-risk gaps from the test audit (73% -> 79% line coverage, 448 py + 46 js tests): - test_fetch_api.py: the POST /api/fetch job machinery - request forwarding, job lifecycle, error mapping (Exception vs SystemExit), live progress visibility, cache refresh, per-job isolation, and the SSE frame protocol PullControl's EventSource parses. This was the most stateful code in the service with zero coverage. - test_mdsthin_fetch.py: the DIII-D _fetch_mdsthin body against a stub mdsthin.Connection - getMany batching/ordering, the per-channel fallback, window/stride TDI suffixes, transport-vs-no-data classification (the #63 chain through the real writer), and per_channel mode. - test_slcontour_fit.py: gaussian-point/-integral bases value-pinned against direct numerical integration (quad/dblquad), including periodic wrapping and the ridge (eps=inf) cases; the mixed-mode sinusoidal-integral pinned as the exact area-average (the site of the deliberate divergence from OMFIT's buggy expression). - test_qs_prep_branches.py: the linear/endpoints detrend estimators (previously dead zones despite being GUI options) and the DIII-D 2019 ESLD66M079/319 wiring-swap branch, both directions. - test_remote_orchestration.py: run_remote's command construction with subprocess faked - jump selection on/off-site, per-user /tmp stage, the remote fetch argv, rsync copy-back target, cleanup, teardown, and fail-fast on a dead master connect. - gui: PullControl's per-device defaults extracted to a pure lib/deviceDefaults.ts helper with vitest cases per device shape (closes #64's core ask). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regression guard, not a target: the suite sits at ~79%, and the headroom covers the deliberately-untestable surfaces (live transports, notebook plotting). Raise as coverage grows; never lower to merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
E741/E402/F541 in the new test files slipped past the local gate run (ruff check was skipped on the last files before push). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
A multi-agent review of the codebase (core, QS pipeline, data/fetch, service, GUI, tests) surfaced three critical bugs and a long tail of majors. This branch fixes them, adds regression tests for each, then fills the highest-risk coverage gaps found by a follow-up test audit (73% → 79%, with a CI floor at 75%).
Critical physics/data fixes
round(phase/Δφ)never wrapped Δφ to its principal value, and the pair picker chose the widest separation (320° on real shots). Wrapped in core + alias-safe pair selection. Verified on shot 174446: the n-map now shows n = 1…5 (was all-zero).Major fixes
sigma_overridecorruptedfit_signal/residual/χ² (de-normalized by the wrong σ);fit_conddefault restored to OMFIT's 1e3 inversion cutoff (10 was the K-trust threshold, silently truncating basis directions).tree_signals(Ip/Bt/κ) were silently never fetched; pointname alias collisions silently dropped a channel; transient transport failures poisoned the incremental cache (closes Fetch: distinguish connection-error from no-data channels (dropped tunnel poisons the incremental cache) #63).refresh()now clears the channel→θ cache; QS ±1σ bands moved from untypedmeta.sigmato the contract'slower/upper(exports now match the screen);channel_usageno longer marks consumed channels droppable; NaN samples serialize as JSON null instead of 500ing.Tests, CI, docs
test_nodes.pyran against the wrong device via a string-sort accident (its family-mixing guard was tautological and the contour test permanently skipped); CI no longer treats "no tests collected" as success;pexpectdeclared.POST /api/fetchjob/SSE machinery, the DIII-D mdsthin fetch body (stub connection),run_remotecommand construction (fake subprocess), the gaussian fit bases (value-pinned vs numerical integration), thelinear/endpointsdetrend estimators, and the ESLD-2019 swap. PullControl's device defaults extracted to a pure tested helper (closes GUI: unit-test PullControl device-aware logic #64's core ask)._slcontourreferences, closed issues dropped) plus a "Running the tests" section.Notes for reviewers
kstar.jsonalias\MC1T10 → \MC1P03(since shot 17377) intentional? It points a toroidal-array position at a poloidal sensor's node.b ∝ cos(nφ − ωt)(rotating toward +φ) reports +n everywhere; the n-map still displays |n| by design.allow_origins=["*"]; unbounded_JOBS; SSH ControlMaster teardown race.Test plan
uv run pytest— 448 passed, 4 skipped (offline, deterministic)uv run ty check src/magnetics,ruff format --check, eslint, tsc — cleancd gui/web && npm run test— 46 passed🤖 Generated with Claude Code