Measured Z_eff/dilution uncertainty: carbon-propagated > VB-measured > scalar envelope ladder - #43
Open
d-burg wants to merge 4 commits into
Open
Measured Z_eff/dilution uncertainty: carbon-propagated > VB-measured > scalar envelope ladder#43d-burg wants to merge 4 commits into
d-burg wants to merge 4 commits into
Conversation
…ss-check
The IDA files carry a measured Zeff uncertainty in BOTH modern vintages --
posterior samples (ensemble layout: 1024 Zeff samples, spread ~7.7 % median
in-core on 169510) and a Zeff_err dataset (newer direct layout: 8.1 % /
9.2 % median on 204441 / 179633) -- which the pipeline discarded in favour
of an assumed 5 % scalar. Since ni is derived from (ne, Zeff) per draw,
that scalar set the width of every ni band ~1.6-1.9x too narrow wherever a
measured envelope existed.
- read_ida: IDAProfiles gains sigma_Zeff + sigma_Zeff_source
("ensemble-samples" | "Zeff_err" | "none"). Older direct files without
Zeff_err report None, NOT a guessed scalar, so the resolver can say which
tier it used.
- resolve_zeff_envelope (baseline.py, unit-tested): the ladder
measured-IDA > zeff_scalar_sigma * |Zeff|, controlled by
UncertaintyConfig.zeff_sigma_source ("auto" default / "measured" /
"scalar" = pre-1.3.2 behaviour). The measured tier is eligible ONLY when
the Zeff baseline itself is the IDA one: on the IMAS/ida_hybrid path the
baseline is FUSE's, and pairing a FUSE baseline with an IDA envelope
would mix channels. "measured" on an incapable file warns loudly and
falls back; the winning tier is logged per run.
- Zeff-vs-carbon cross-check (report-only, one line per read): the file
reports Zeff from visible bremsstrahlung AND n_12C6 from CER; under the
same single-impurity assumption they must agree as 1 + Z(Z-1) nC/ne
(Callahan 2019 JINST 14 C10002 is the agreement pedigree when C6+
dominates). Measured core-medians on the demo shots: -1.7 % (169510),
+4.7 % (204441), +11.3 % (179633) -- shot-dependent and mostly within
the file's own measured sigma_Zeff, so the config.py blanket comment
"IDA's Zeff is unreliable" is softened to the per-shot statement.
RESULTS-CHANGING, per maintainer approval: with "auto" (default), cases
whose files carry a measured envelope get wider Zeff/ni bands (~8-9 %
median vs 5 %). Old files and the IMAS path are bit-unchanged; "scalar"
restores the previous behaviour exactly.
13 new solve-free tests: synthetic .cdf fixtures for all three vintages,
the full ladder including the FUSE-baseline gate and both loud fallbacks,
and the carbon cross-check on consistent and deliberately-inconsistent
fixtures. Fast suite 389+13 passed.
Collaborator finding (S. Benjamin), verified here: the file-reported
Zeff_err is enormous outside the core -- grand means 19-35 % on the demo
direct files (~90 % reported on some shots), driven by a SOL tail measured
at 44-130 % of Zeff -- because the VB inversion's error carries
n_e^2 sqrt(T_e) propagation, calibration and mantle-subtraction
systematics. The dilution the Zeff-primary scheme actually perturbs
(ni = ne - Z nC) is DIRECTLY measured by CER carbon, and propagating
(n_12C6_err, n_e_err) through it gives a dramatically tighter, better
behaved envelope: 1.9-5.8 % of Zeff in-core, 4-19 % SOL, on the same
files. Drawing Zeff with that sigma IS error propagation through
ni = ne - Z nC, since ni is an exact function of the drawn (ne, Zeff) --
so this implements the collaborator's proposal inside the existing scheme,
with no new assumption (single-impurity C6+ dominance is already the
scheme's premise, and read_ida's Zeff-vs-carbon line flags files where it
creaks).
Ladder is now: carbon-propagated > VB-measured > scalar, under
zeff_sigma_source "auto" | "carbon" | "measured" | "scalar", with loud
fallbacks and a report-only warning when the chosen envelope's median
fraction exceeds 25 % of |Zeff| (never clipped -- flagged).
Also fixes a real eligibility bug the 179633 end-to-end A/B caught: the
measured tiers were gated on `src_aux.get("zeff") is None`, but the
RECONSTRUCTION path also populates aux['zeff'] (it IS the IDA Zeff, stored
for the aux plots), so every recon-path run silently resolved to the
scalar. Eligibility is now decided by the source type (ReconstructionSource
with an active IDA path); the IMAS/ida_hybrid path stays scalar-only as
designed.
Reader: sigma_Zeff_carbon (+ source) computed on both layouts --
n_12C6_err propagation on direct files, the dilution posterior
1 + Z(Z-1) nC_s/ne_s on ensemble files. Measured on the demo shots:
169510 5.8 % (ensemble), 204441 2.5 %, 179633 1.9 % core median.
19 solve-free tests (was 13): carbon tiers on both layouts, ladder
preference order, forced-mode fallbacks, the implausible-size warning, and
the report-only (not clipped) contract. Fast suite 408 passed.
…nce, drop dead test code - baseline.py / io/ida.py: three statements each had lost their line wrapping (100+ char lines with embedded whitespace runs); rewrapped in parentheses, no behaviour change. - baseline.py: the end-to-end A/B comment now says 'a demo shot' instead of naming a DIII-D shot number (public repo). - test_zeff_sigma_ladder.py: remove an inert empty h5py.File block that _write_direct truncated anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ire impurity_Z, tighten IDA eligibility Four adversarial-review findings: - Direct-layout carbon tier: negative n_12C6 (SOL spline undershoot), netCDF fill values, and NaN holes all survived the clip floors -- a negative nC keeps its sign while its magnitude is floored out of the relative-error denominator, producing ~1e6-scale sigmas that passed every downstream guard and silently corrupted the Zeff/ni ensemble (reproduced). Any non-physical radius now drops the tier with a printed reason (matching the None-not-a-guessed-scalar rule); the nan_to_num that converted NaN holes into never-perturbed sigma=0 radii is gone with it. - Zeff-vs-carbon cross-check: an edge-only fit vintage (no psi_N <= 0.9) crashed read_ida via argmax([]) inside the report-only check (reproduced); the mask now also requires valid carbon/ne points and the check skips loudly when none remain. - resolve_uncertainty's sigma-envelope read_ida call now passes source.impurity_Z (the carbon propagation is Z(Z-1)-quadratic; it was pinned to Z=6 for every machine while the kinetics loader passed the real value). - Measured-tier eligibility now requires the sigma .cdf to BE the baseline's own profiles file: a p-file Zeff baseline with unc.ida_path set, or unc.ida_path naming a different file/vintage than the source's own .cdf, previously paired a foreign absolute envelope with a Zeff it never measured. (Also drops a dead alias left from the abandoned aux-dict gate.) 8 new tests: garbage-carbon tier drops (negative / fill / NaN-hole), edge-only-grid crash guard, and resolve_uncertainty-level wiring for all three eligibility arms plus the impurity_Z pass-through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness issues in the new ladder implementation (sigma validation and path-equivalence gating) that can cause invalid envelopes or silent scalar fallback.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the reconstruction/IDA uncertainty pipeline so the Z_eff perturbation envelope is sourced from the highest-fidelity tier available in each IDA file (carbon-propagated dilution > VB-measured > scalar), rather than always using a fixed scalar fraction.
Changes:
- Extend
read_ida()to surface measuredsigma_Zeffand carbon-propagatedsigma_Zeff_carbon, plus a report-only Zeff-vs-carbon QC metric. - Add
UncertaintyConfig.zeff_sigma_sourceand implement the Z_eff envelope “ladder” inresolve_zeff_envelope(), wiring it intoresolve_uncertainty(). - Add comprehensive solve-free tests covering file vintages, ladder behavior, and gating rules.
File summaries
| File | Description |
|---|---|
| tests/test_zeff_sigma_ladder.py | New fixtures/tests for IDA sigma tiers, ladder preference, wiring/gating, and QC behavior. |
| bouquet/io/ida.py | Adds measured Zeff sigma + carbon-propagated sigma computation and report-only Zeff-vs-carbon cross-check. |
| bouquet/config.py | Documents and introduces UncertaintyConfig.zeff_sigma_source ladder configuration. |
| bouquet/baseline.py | Implements resolve_zeff_envelope() and wires ladder selection into resolve_uncertainty(). |
Review details
Suppressed comments (1)
bouquet/baseline.py:237
- The warning text in
_usable()is hard-coded to "measured sigma_Zeff" but the same helper is used for the carbon-propagated tier as well, so the message can be misleading. Also, once negative values are rejected, the warning should mention "negative" explicitly to match the validation.
warnings.warn(
f"resolve_zeff_envelope: a measured sigma_Zeff is unusable "
f"(shape {a.shape} vs {base.shape}, or non-finite/all-zero); "
f"tier skipped", stacklevel=3)
- 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
+231
to
+233
| a = np.asarray(m, dtype=float) | ||
| if a.shape == base.shape and np.all(np.isfinite(a)) and np.any(a > 0): | ||
| return a |
Comment on lines
+464
to
+468
| _src_cdf = (str(getattr(src, "profiles_path", "")) | ||
| if isinstance(src, ReconstructionSource) else "") | ||
| _zeff_baseline_is_ida = (ida_path is not None | ||
| and _src_cdf.endswith(".cdf") | ||
| and ida_path == _src_cdf) |
Comment on lines
+7
to
+11
| reports the highest-fidelity tier the file can support, and | ||
| ``resolve_zeff_envelope`` picks it with explicit provenance: | ||
|
|
||
| measured IDA sigma_Zeff > zeff_scalar_sigma * |Zeff| | ||
|
|
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.
Results-changing by design, per maintainer approval: on the reconstruction/IDA path, the Z_eff envelope (which sets the width of every derived-
niband) now comes from the highest-fidelity tier the file supports instead of an assumed 5 % scalar.The two findings this implements
Zeff_errdataset (newer direct layout) — which the pipeline discarded.Zeff_erris a poor dilution error: 8–9 % of Zeff in-core but 44–130 % in the SOL (grand means to ~90 % on some shots), because the VB inversion carries nₑ²√Tₑ propagation, calibration and mantle-subtraction systematics. The dilution the Zeff-primary scheme actually perturbs (ni = ne − Z·nC) is directly measured by CER carbon: propagating(n_12C6_err, n_e_err)gives 1.9–5.8 % core / 4–19 % SOL on the same files. Drawing Zeff with that σ IS error propagation throughni = ne − Z·nC(ni is an exact function of the drawn pair), so this is the collaborator's proposal inside the existing scheme, with no new assumption — single-impurity C6+ dominance is already the scheme's premise, and a new per-file Zeff-vs-carbon QC line (Callahan 2019 JINST 14 C10002 pedigree) flags files where it creaks: measured −1.7 / +4.7 / +11.3 % core-median on the three demo shots.The ladder
UncertaintyConfig.zeff_sigma_source = "auto" | "carbon" | "measured" | "scalar"; auto = carbon-propagated > VB-measured > scalar, loud fallbacks, report-only warning (never a clip) when a chosen envelope's median fraction exceeds 25 % of |Zeff|. Old files without carbon/Zeff errors fall back with the tier logged;"scalar"reproduces pre-change behaviour exactly. The measured tiers are eligible only on the reconstruction path — the IMAS/ida_hybrid Z_eff baseline is FUSE's and is never paired with an IDA envelope.End-to-end validation (179633, 3-draw seeded runs per arm)
n_12C6_err)Zeff_err)Each arm logs its provenance (
[sigma] zeff envelope <- …). Net effect of the default: ni bands ~2× tighter than the old 5 % scalar on these shots — measured, in whichever direction it goes.A bug the end-to-end A/B caught
The first eligibility gate tested
src_aux.get("zeff") is None, but the recon path also storesaux[zeff](it is the IDA Zeff, kept for the aux plots) — so every recon-path run silently resolved to the scalar. Now gated on the source type. This is why the A/B table above exists: the unit tests alone passed with the bug present.Tests / suites
19 solve-free tests: synthetic
.cdffixtures for all three vintages, carbon tiers on both layouts, ladder preference order, the FUSE-baseline gate, forced-mode loud fallbacks, the implausible-size warning (report-only, not clipped), and the carbon cross-check on consistent and deliberately-inconsistent fixtures. Fast suite 408 passed; systematics goldens 3 passed and seeded solver suite 12 passed (both bit-unaffected — the golden fixture is peqdsk-based and takes the scalar path).Refs #19 (adjacent Zeff bookkeeping). Measured values quoted from IDA_169510_/IDA_204441_/IDA_179633_ demo files.