Refactor quasistationary code into magnetics.core#51
Open
priyanshlunia wants to merge 5 commits into
Open
Conversation
Retire the dead core/quasistationary.py (a partial, unwired port whose
reconstruct_grid used exp(+i(nφ+mθ)) — wrong for any phased mode) plus its
falsely-certifying test, and promote the *active* SLCONTOUR pipeline into core
under unified qs_ naming, deleting the omfit_compat OMFIT shim.
- Move _slcontour/{fit,prep,io_data,run,plots}.py -> core/qs_*.py.
- Delete omfit_compat.py, folding each symbol to its owner: printv/w/e -> logging;
OMFITexception -> ValueError; delta_degrees -> qs_fit; cornernote/uband ->
qs_plots; device readers -> new core/qs_device.py, which delegates to the data
layer (data.devices / data.diiid_geometry) and keeps only the QS-specific extras
(derived theta/end-coord sensor_geometry, resolve_channel_filter semantics).
- qs_device.load_wall now delegates to devices.feature_at, fixing the segmented-
schema bug (the shim read a nonexistent top-level `wall` key -> (None, None)).
- Lock the -i reconstruction convention: invariant comments at the three sites
+ a discriminating δ≠0 regression test the old zero-phase test missed.
- qs_plots docstring clarified as standalone matplotlib (not GUI/service wired).
- Rewire service/nodes.py + tests; pyproject drops the ruff exclude and retargets
a deferred ty exclude onto the qs_* files. Relocate the reference notebook/README
to docs/qs_slcontour/.
ruff format/check + ty (with service extra) clean; 209 passed, 1 skipped.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Finish the core/qs_* promotion by cleaning up its reference material, which was
still written for the old OMFIT `magnetics-code` layout.
- Move example_magnetics.ipynb into examples/ and rewire it onto the new
infrastructure: imports become magnetics.core.qs_{io_data,device,run} plus
`qs_plots as plots` (so the plot cells are unchanged); drop the sys.path hack
and the OMFIT-script mapping / "ported OMFIT scripts" framing.
- Replace docs/qs_slcontour/README.md with docs/quasistationary-mode-analysis.md:
no OMFIT-magnetics plumbing (module↔OMFIT-script table, omfit_compat, cd
analysis), instead a walkthrough of the qs_* pipeline (io_data → prep → fit →
run, plus qs_bridge→service and standalone qs_plots), data inputs, quick start,
and the -i reconstruction sign convention. Physics provenance (SLCONTOUR,
VISION §4.1) kept.
- Remove the now-empty docs/qs_slcontour/.
Docs-only; no src/ behavior change. ruff format --check clean.
(docs/magnetics-code-architecture.md is also OMFIT-era — left for a later sweep.)
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR consolidates the quasi-stationary (SLCONTOUR-style) analysis into magnetics.core under a qs_* namespace, retiring the unused pure-numpy quasistationary.py port and removing the _slcontour/omfit_compat shim, while updating the service, tests, and docs to point at the promoted core pipeline.
Changes:
- Promote the active SLCONTOUR pipeline into
src/magnetics/core/qs_{io_data,prep,fit,run,device,plots}.pyand keepqs_bridgeas the service/GUI adapter. - Remove the dead
src/magnetics/core/quasistationary.pypath and the_slcontourshim/docs. - Rewire service + tests + docs to the new modules and adjust tooling excludes in
pyproject.toml.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_slcontour_geometry.py | Update imports/docs to validate geometry via core.qs_device instead of _slcontour.omfit_compat. |
| tests/test_slcontour_fit.py | Point basis-function tests at core.qs_fit and update expected exception type. |
| tests/test_quasistationary.py | Remove tests for the retired core/quasistationary.py port. |
| tests/test_qs_pipeline.py | Update QS end-to-end test wiring to qs_* modules. |
| tests/test_qs_bridge.py | Add regression coverage for the reconstruction sign convention using a non-zero spatial phase. |
| src/magnetics/service/nodes.py | Switch QS pipeline orchestration to core.qs_run.run_steps and wall loading to core.qs_device.load_wall(device, shot). |
| src/magnetics/core/quasistationary.py | Delete the unused pure-numpy QS port. |
| src/magnetics/core/qs_run.py | Ensure QS orchestrator imports qs_fit/qs_prep/qs_io_data modules. |
| src/magnetics/core/qs_prep.py | Replace OMFIT shim prints with stdlib logging; keep prep logic in core. |
| src/magnetics/core/qs_plots.py | Replace OMFIT shim plot helpers with local matplotlib helpers; keep sign convention comments. |
| src/magnetics/core/qs_io_data.py | Repoint loader dependencies to qs_device and replace shim prints with logging. |
| src/magnetics/core/qs_fit.py | Replace OMFIT exception type + logging changes; (contains a Python-syntax issue noted in comments). |
| src/magnetics/core/qs_device.py | New device adapter delegating to magnetics.data.* for segment resolution/sets; (contains Python-syntax issues noted in comments). |
| src/magnetics/core/qs_bridge.py | Update docs/comments to reflect qs_fit provenance; pin reconstruction sign convention with invariant comments. |
| src/magnetics/_slcontour/README.md | Remove deprecated reference README. |
| src/magnetics/_slcontour/omfit_compat.py | Remove OMFIT compatibility shim (replaced by qs_device + logging). |
| src/magnetics/_slcontour/init.py | Remove deprecated _slcontour package init. |
| pyproject.toml | Drop ruff exclude for _slcontour; add targeted ty excludes for qs_* modules. |
| examples/example_magnetics.ipynb | Rewire notebook to import/run magnetics.core.qs_* directly (no sys.path hacks). |
| docs/specs/quasistationary-mode-analysis.md | Add updated pipeline spec, inputs, and explicit reconstruction sign convention documentation. |
Comments suppressed due to low confidence (1)
src/magnetics/core/qs_fit.py:334
- Invalid Python exception syntax:
except ValueError, np.linalg.LinAlgError:will raise a SyntaxError and prevent this module from importing. Use a tuple of exception types instead.
Comment on lines
+122
to
+126
| def _to_float(s): | ||
| try: | ||
| return float(s) | ||
| except TypeError, ValueError: | ||
| return float("nan") |
Comment on lines
+203
to
+207
| """ | ||
| try: | ||
| dev = load_device(device) | ||
| except FileNotFoundError, OSError, ValueError: | ||
| return None, None |
Brings develop through #46 (gui-sensors): NSTX/NSTX-U device-generic fetch, per-node plot + HDF5 export, EFIT tree-fetch, and the data-layer device_geom refactor. No merge conflicts; the qs_* pipeline works against develop's refactored data layer. Verified on the merged tree: ruff format/check clean, ty clean, 290 passed / 5 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…core Two device-specific bits removed from the QS core (issue #40 device-agnostic goal), without touching data/device/*.json or the data layer: - qs_device: delete the hardcoded `_DEVICE_ALIAS = {"DIII-D": "diiid"}` and `_device_key`; `load_device` now delegates to `devices.resolve_device_id`, which derives the config id from each device JSON's `name` field. - qs_fit: remove the advisory `is_device("DIII-D")` + `C.*/IL.*/IU.*` coil-basis warning and its now-unused `is_device` import. The 2019 ESLD wiring swap in qs_prep is intentionally kept for now (migrating it needs a JSON `corrections` block + reader, deferred). ruff/ty clean; 290 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
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.
Retire
quasistationary.py; promote the SLCONTOUR pipeline into shim-freecore/qs_*Summary
Reverses the direction of #40: instead of finishing the half-built pure-numpy
core/quasistationary.pyport, this retires that dead module and promotes theactually-served SLCONTOUR quasi-stationary pipeline (
_slcontour/) intocore/under aunified
qs_namespace, deleting theomfit_compatOMFIT shim entirely. Net −356 lines(635 insertions / 991 deletions).
Three commits, each self-contained:
refactor(core): promote SLCONTOUR pipeline into shim-free core/qs_*— the move + shim removal.docs: relocate + modernize the QS reference notebook & README— reference-material cleanup.Adjusted stale docstring references, clarified each script's context and role— docstring pass.Why
An audit turned up two facts:
core/quasistationary.pywas dead weight — a partial pure-numpy port of only the fit step(sinusoidal only; no prep, no loader, no Gaussian/vertical geometry), not wired into
production, and its
reconstruct_gridusedexp(+i(nφ+mθ)), which mirrors the toroidalphase / flips helicity and is wrong for any mode with a non-zero spatial phase. It even
shipped a test that falsely certified it, because the test only exercised zero-phase modes
where
+iand−ihappen to agree._slcontour(io_data → prep → fit) driven throughcore/qs_bridgeis what the service serves, and its−ireconstruction matches the originalDIII-D SLCONTOUR reference.
So the maintainable move is to make the working pipeline first-class and delete the misleading port.
What changed
Retire the dead port
core/quasistationary.pyand its degenerate test.Promote
_slcontour→core/qs_*(shim-free)_slcontour/{fit,prep,io_data,run,plots}.py→core/qs_{fit,prep,io_data,run,plots}.py.omfit_compat.py, folding each symbol to its owner:printv/w/e→ stdliblogging;OMFITexception→ValueError;delta_degrees→qs_fit;cornernote/uband→qs_plots.core/qs_device.py— the shared device-metadata leaf. It delegates to the data layer(
data.devices,data.diiid_geometry) and keeps only the QS-specific extras: the derivedtheta/*_end1/2sensor_geometry, theresolve_channel_filtersemantics, and the"DIII-D" → diiidalias.data/is left untouched.Lock the reconstruction sign convention
qs_bridge._reconstruct_grid,qs_bridge.fit_to_phi_t_node,qs_plots.plot_slice) stating: fit basisexp(+i(nφ+mθ))with areal/imag column split ⇒ reconstruction must use
exp(−i(…)).tests/test_qs_bridge.py(non-zero spatial phase δ)that fails against
+iand passes against−i— the test the old one lacked.Fixes a latent bug
qs_device.load_wallnow delegates todevices.feature_at("first_wall"); the old shim read anon-existent top-level
wallkey and returned(None, None)for every shipped device, so the QSsensor-map wall silently never rendered. It now returns the real outline (
nodes.pythreadsshot).Rewire + config + docs
service/nodes.py→core.qs_run.run_stepsandcore.qs_device.load_wall.pyproject.toml→ drop the ruff exclude (theqs_*files are ruff-clean); a targetedtyexcludeis retained on the
qs_*files as a documented follow-up (see below).qs_plots.pydocstring reframed as standalone matplotlib (verified: nothing in the served pathimports it —
qs_bridgeonly ports its conventions).examples/example_magnetics.ipynb, rewired ontomagnetics.core.qs_*(no more
sys.path/omfit_compatimports). README rewritten asdocs/specs/quasistationary-mode-analysis.md(no OMFIT-magnetics plumbing).qs_*recontextualized (OMFIT script provenance + pipeline step).Verification
uv run ruff format --check .— clean.uv run ruff check .— clean.uv run --extra service ty check src/magnetics— clean.uv run --extra service pytest— 209 passed, 1 skipped (the 3-test drop is the deleteddegenerate
quasistationarysuite; the new sign-convention regression test is included).Follow-ups (intentionally out of scope)
qs_*filesty-clean and drop their targetedtyexclude.data/againstqs_device.pure-numpy
corefit.docs/magnetics-code-architecture.mdis still OMFIT-era and stale — later docs sweep.Reviewer notes
load_wallfix lightly touches the Sensors/geometry surface (Data Streamers / Interfacersheads-up): it now returns a non-empty wall where it previously returned
(None, None).data/files were modified;qs_deviceonly imports from the data layer.🤖 Generated with Claude Code