Conversation
SolverConfig.coil_reg was declared (config.py:66) and never read. Both places that install coil regularisation hardcoded target=0.0 at unit weight: setup_solver and _reset_solver_state. That second one is the trap. prepare_baseline calls _repoint_imas_geometry -> _reset_solver_state immediately before the IMAS baseline solve, so targets installed after setup_solver were silently discarded. Measured on 174823: a target of 1e6 A-t at weight 1e8 moved F6A by 0.2% (-175904 vs -175602 unconstrained) and in the wrong direction -- not a weight problem, the terms simply were not present when the solve ran. Both sites now route through _apply_coil_reg, which uses config.solver.coil_reg when populated and otherwise reproduces the old behaviour exactly. Coils not named in the spec keep a zero target, and the weak #VSC term is preserved. bouquet/coil_targets.py builds the spec from measured currents, converting circuit amps to TokaMaker ampere-turns with EFIT dprobe TURNFC (58 for F1-F5/F8, 55 for F6/F7/F9). E-coils convert at 1.0 because the shipped D3D mesh carries their turns itself -- coil_dict nturns sums to exactly 61.0 for ECOILA/ECOILB, matching the /61.0 divisor in the OFT DIII-D example. 8 tests, including one asserting the default path is unchanged when coil_reg is empty, and one pinning TURNFC against dprobe.
coil_reg_term raises KeyError on an unknown coil name, so a spec naming a coil absent from the mesh killed setup_solver outright. A measurement source is not mesh-specific: DIII-D pf_active carries all 24 circuits (ECOILA/B, E567UP/DN, E89UP/DN, F1A-F9B) while the shipped D3D mesh models 20 coil sets -- the four E567/E89 circuits are missing -- so every measured-target run crashed with KeyError: 'Unknown coil "E567UP"'. Unmatched terms are now dropped with a UserWarning naming them, rather than failing the solve. 398 tests.
Distinct from coil_reg. coil_reg puts the measured currents in the OBJECTIVE, so the solve is pulled toward them throughout and boundary fit is traded away (2.34 -> 4.94 mm at full pinning). coil_init puts them in the INITIAL ITERATE: the under-relaxed Picard iteration starts in the chosen basin of the degenerate coil manifold, and nothing in the objective fights the boundary. Applied inside _forward_solve_imas_baseline AFTER init_psi, which reinitialises coil currents from the regularisation and would overwrite an earlier set -- the same discard trap that hid the coil_reg targets. Coils absent from the mesh are dropped; unlisted known coils keep their value. Unset => coils start at zero, the historical behaviour. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…/coil-reg-targets
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core solver setup/reset behavior for baseline solves and should be validated with domain-aware review and runtime testing on representative IMAS cases.
Pull request overview
This PR makes SolverConfig.coil_reg an active, opt-in mechanism to regularize the baseline inverse solve toward measured coil currents, ensuring the configured regularization survives solver resets (notably before the IMAS baseline solve). It also introduces SolverConfig.coil_init as an optional seed for the IMAS baseline inverse iteration and adds a helper module to build coil-regularization targets from pf_active using device-registry turns.
Changes:
- Add
Bouquet._apply_coil_reg()and call it from bothsetup_solver()and_reset_solver_state()so coil regularization targets persist across solver resets. - Add optional
SolverConfig.coil_initseeding in_forward_solve_imas_baseline()afterinit_psi. - Introduce
bouquet.coil_targets(turns alias +coil_reg_from_measured+measured_from_pf_active) and add focused tests for coil targets and coil-reg application.
File summaries
| File | Description |
|---|---|
bouquet/run.py |
Centralizes coil-regularization installation and re-applies it on solver reset; adds optional IMAS baseline coil seeding. |
bouquet/config.py |
Documents and exposes new solver config fields coil_reg and coil_init. |
bouquet/coil_targets.py |
New helper utilities for building coil regularization specs from measured currents and device-registry turns. |
tests/test_coil_targets.py |
Adds tests covering turns aliasing, coil-reg spec construction, and _apply_coil_reg behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+379
to
+388
| spec = list(getattr(self.config.solver, "coil_reg", None) or []) | ||
| if spec: | ||
| # Drop terms naming coils this MESH does not model. A measurement | ||
| # source is not mesh-specific: DIII-D pf_active carries all 24 | ||
| # circuits while the shipped D3D mesh models 20 coil sets (no | ||
| # E567UP/E567DN/E89UP/E89DN), and coil_reg_term raises KeyError on | ||
| # an unknown name -- which would kill setup_solver outright. | ||
| known = set(mygs.coil_sets) | {"#VSC"} | ||
| skipped = sorted({c for t in spec for c in t["coils"]} - known) | ||
| spec = [t for t in spec if set(t["coils"]) <= known] |
Comment on lines
+667
to
+670
| _ci = getattr(self.config.solver, "coil_init", None) | ||
| if _ci: | ||
| _known = set(mygs.coil_sets) | ||
| _use = {k: float(v) for k, v in _ci.items() if k in _known} |
Comment on lines
+118
to
+122
| def test_turns_come_from_the_device_registry(): | ||
| from bouquet.coil_targets import TURNFC_D3D | ||
| from bouquet.devices import get_device | ||
| assert TURNFC_D3D == get_device("DIII-D").turns | ||
| assert TURNFC_D3D["F1A"] == 58.0 and TURNFC_D3D["F6A"] == 55.0 and "ECOILA" not in TURNFC_D3D |
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.
Stacked on #49.
SolverConfig.coil_regbecomes real: regularisation targets for the baseline inverse solve, applied in bothsetup_solverand the solver reset before the IMAS baseline solve, withcoil_targets.coil_reg_from_measured()building them frompf_active(turns from the device registry). Coils absent from the mesh are skipped with a warning.Why: an unregularised TokaMaker baseline sits a median 11.6 kA-t from the measured coil currents, ~10× EFIT's own offset and uncorrelated with it, along the coil null space. Draw filters judge plausibility relative to the baseline and cannot see this. With inverse-variance weights w_i ∝ W0/σ_i² and W0 = 100 the baseline is within ~2σ of the machine on every L-mode and low-β_N slice checked, at nil boundary cost in L-mode and 1.6–3.8 mm in H-mode (growing with β_N). Recommended rule: W0 = 100 for β_N ≲ 0.5, W0 = 10 above; never W0 ≲ 5 (a weak pull resolves the inner/mid coil null space the wrong way). Opt-in in this PR; making it the default is a follow-up decision.
coil_init(seeding the inverse iterate) is included but off by default: coil currents are re-solved every Picard step, so seeding has no effect.🤖 Generated with Claude Code