Skip to content

Tearing - BUGFIX - Stop the false chi_e fallback warning under chi_perp_Dc and test its forwarding - #619

Merged
logan-nc merged 6 commits into
developfrom
bugfix/chi-perp-dc-followups
Nov 2, 2026
Merged

logan-nc merged 6 commits into
developfrom
bugfix/chi-perp-dc-followups

Conversation

@d-burg

@d-burg d-burg commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Release note

  • Audience: users
  • Numerical impact: none; a warning, documentation and tests (harness @ 62540fe)
  • Migration: none

A run that sets chi_perp_Dc no longer warns about a chi_e fallback it does not use, and setting the key with D_c_type = "none" now warns that it has no effect. The docs now say that the deck key is one value for every surface and that it also moves the critical resonant field.

Regression report

regress --refs 728a7e7d5,62540feda --force (all cases) on feynman (SLURM, 8 threads, Julia 1.11.9, manifest pinned): every case is unchanged, all 25; gal_resistive_pe is N/A on both refs, as on develop. Later commits touch only docs/src/stability.md.

On the Mac (Julia 1.11.6) at 62540feda: runtests_slayer_inputs.jl 94/94, runtests_slayer_runner.jl 171/171.

Notes for reviewers

Follow-up to #616, from an adversarial review of it that finished after it merged.

  • False warning. _slayer_chi_inputs counted χ⊥,e as used by the critical Δ even with chi_perp_Dc set. With P_perp_model = "tau_E", a non-none D_c_type, chi_perp_Dc set and no chi_e in the file, the run warned about a fallback that is never used.
  • No-effect warning. chi_perp_Dc with D_c_type = "none" was silently ignored.
  • Docs. SLAYERControl and stability.md now say the deck value applies at every surface, over any chi_e profile (the ψ-callable form exists only on build_slayer_inputs), that it is positive and finite, and that it reaches the critical resonant field through the critical Δ. The motivation in stability.md now treats the value as a modeling choice and names the chi_e/chi_i closure that reaches the same split, and where that fails.
  • Which χ⊥ the sources mean. stability.md said the critical Δ takes the electron channel "in every case". The sources are single-fluid: Fitzpatrick, Phys. Plasmas 32, 062509 (2025), defines the width by electron temperature but with one χ⊥, and Connor et al. (2015) Eqs. 59 and 61 diffuse the total pressure with it. The docs now say the electron channel is GPEC's reading. The code is unchanged.
  • Tests. The forwarding through _tearing_params was untested: removing the chi_perp_Dc=chi.chi_perp_Dc line left every test green. The new test fails with that line removed (checked by hand). The ψ-callable form is now covered too, and the three warning paths.

Not in this PR: a per-surface chi_perp_Dc deck table, like omega_E_kHz.

For the CHANGELOG: #616 merged with a ! in its title and Migration: none. Nothing in it breaks an existing deck, so it should read as a plain feature.


🤖 Generated with Claude Code

…rp_Dc and test its forwarding

With chi_perp_Dc set the critical Δ no longer uses χ⊥,e, so a run whose P_perp model does not use it either should not warn about a chi_e fallback. Setting the key with D_c_type = none now warns that it has no effect. Documents that the deck key is one value for every surface and that it reaches the critical resonant field, and tests the callable form and the forwarding through the TearingProblem assembly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@d-burg d-burg added this to the GPEC v2.0.0 milestone Oct 30, 2026
@d-burg d-burg self-assigned this Oct 30, 2026
@d-burg
d-burg requested a review from matt-pharr October 30, 2026 18:30
@github-actions github-actions Bot added the bugfix Something was wrong and now is not label Oct 30, 2026
d-burg and others added 3 commits October 30, 2026 16:38
The critical Δ still needs an electron diffusivity when P_perp uses a one-fluid χ_eff, so the
key states that choice rather than implying the power balance supplies the electron χ. Names the
chi_e/chi_i closure that reaches the same split, and where it fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s a modeling choice

The W_d and critical-Δ sources are single-fluid, so the electron channel is GPEC's reading of a
single χ_⊥ rather than a statement those papers make.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@logan-nc

logan-nc commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator

@d-burg @matt-pharr We're taking this over to carry it to the merge, since #602 landed and this now conflicts with develop. I'm merging develop in and pushing shortly so CI starts, then doing a full review and re-running the harness. Please hold off pushing to this branch until I post the follow-up.

…_Dc warnings to SLAYERModel

#602 replaced SLAYERControl with SLAYERModel and moved the chi inputs to Tearing/SLAYERTearing.jl: the fallback and no-effect warnings, the docstring and the tests move with them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186CLfjeJeSusNTva5RMfGU
Copilot AI balanced review requested due to automatic review settings November 1, 2026 23:45
@logan-nc

logan-nc commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator

Pushed 6c0a31ad3: develop merged in, with the change ported to where #602 moved it.

  • The fallback and no-effect warnings now live in Tearing._slayer_chi_inputs and act on SLAYERModel.
  • The chi_perp_Dc notes moved to the SLAYERModel docstring.
  • The docs paragraph is kept on top of develop's SLAYERModel wording.
  • The tests use the new API: _tearing_params(model, equil, surfaces, loaded), with kinetic data attached for that check.

CI is running. Targeted tests (slayer_inputs, slayer_runner, critical_resonant_field) are running on feynman, and a full review follows.

…perp_Dc replaces it in the critical Δ

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186CLfjeJeSusNTva5RMfGU
@logan-nc

logan-nc commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator

Review follow-up, pushed as be3709294.

  • Same gap, one level down: _slayer_profiles (SLAYER/KineticView.jl) still loaded the file's chi_e whenever there was a critical Δ, and errors on any non-positive value. So a run with chi_perp_Dc set and an interpretive chi_e ≤ 0 stopped on a profile it never uses, which is the case the docs give for this key. It now skips chi_e there, the same way the warning does, and a test covers both sides.
  • Test fix: the ported _tearing_params test now builds its kinetic data from arrays.

Targeted tests are re-running. The full harness follows the #617 run on alfven, and then the stamp gets updated.

@d-burg One question I couldn't settle: the docs paragraph cites Fitzpatrick, Phys. Plasmas 32, 062509 (2025), Eqs. 230–234, and Connor et al., PPCF 57, 065001 (2015), Eqs. 59 and 61. Neither paper is in docs/resources, so I couldn't check the equation numbers. Can you confirm them?

@logan-nc

logan-nc commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator

Targeted tests pass at be3709294: runtests_slayer_inputs 94/94, runtests_slayer_runner 156/156 and runtests_critical_resonant_field 71/71. The full harness (alfven job 860) is running; the stamp follows.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

An active compatibility issue drops a valid chi_e profile and passes nothing to a Real-typed parameter, causing previously runnable inputs to fail and requiring a fix with regression coverage.

1 open issue
What changed in this PR

This follow-up to #616 corrects SLAYER diffusivity warnings and profile handling, adds forwarding tests, and clarifies how chi_perp_Dc affects critical-Δ calculations.

Changes:

  • Suppresses unused chi_e fallback warnings and warns when chi_perp_Dc has no effect.
  • Tests callable inputs, parameter forwarding, and warning paths.
  • Documents the uniform deck override and its physical interpretation.

The PR reports unchanged regression results. Keep the regression-harness report tied to the reviewed commit.

Third-party human approval is required before merging into develop.

File Description
test/​runtests_slayer_runner.jl Tests warnings and unused electron-profile handling.
test/​runtests_slayer_inputs.jl Tests callable overrides and parameter forwarding.
src/​Tearing/​SLAYERTearing.jl Corrects fallback and no-effect warnings.
src/​InnerLayer/​SLAYER/​SLAYER.jl Clarifies override scope and critical-field effects.
src/​InnerLayer/​SLAYER/​KineticView.jl Skips electron profiles when unused.
docs/​src/​stability.md Explains diffusivity modeling choices and override behavior.

Review effort: Balanced

model = control === nothing ? nothing : control.P_perp_model
uses_e = model in (:chi_perp_e, :D_perp) || (control !== nothing && control.D_c_type !== :none)
# With chi_perp_Dc set, the critical Δ takes it in place of χ⊥,e, so only P_perp can need the file's chi_e.
uses_e = model in (:chi_perp_e, :D_perp) || (control !== nothing && control.D_c_type !== :none && control.chi_perp_Dc === nothing)
@logan-nc
logan-nc enabled auto-merge November 1, 2026 23:59
@logan-nc
logan-nc requested a review from jhalpern30 November 2, 2026 00:00
@logan-nc

logan-nc commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

@matt-pharr @jhalpern30 This needs a review from one of you, since we pushed the port and the chi_e fix ourselves. The diff against develop is small: +61/−11 across docs, two lines of warning logic in SLAYERTearing.jl, one condition in KineticView.jl, and tests. Targeted tests pass. The harness (alfven job 860) is running and the stamp goes in when it finishes.

@logan-nc
logan-nc self-requested a review November 2, 2026 00:15
@logan-nc
logan-nc added this pull request to the merge queue Nov 2, 2026
Merged via the queue into develop with commit 961484b Nov 2, 2026
16 of 19 checks passed
@logan-nc
logan-nc deleted the bugfix/chi-perp-dc-followups branch November 2, 2026 00:31
@logan-nc

logan-nc commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Harness record, run after merge: regress --refs e51f4b0ee,be3709294 (all 25 cases) on alfven, 16 threads, Julia 1.11.7, manifest pinned. Every tracked quantity is unchanged. gal_resistive_pe reports N/A on both refs, develop included, so it is a stale case definition and not this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Something was wrong and now is not

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants