Skip to content

Add a multi-n perturbed-equilibrium regression case (+ threaded verification) for singular-coupling threading #334

Closed

Description

@logan-nc

Motivation

PR #325 parallelized the singular-coupling loop in PerturbedEquilibrium/SingularCoupling.jl over resonant (surface, n) pairs (and the NTV / field-reconstruction paths). During review we found that the regression harness has no multi-n perturbed-equilibrium case, and CI never sets JULIA_NUM_THREADS, so:

  • diiid_n1 is single-n (nn = 1).
  • solovev_multi_n is multi-n but ideal-only — it does not run perturbed equilibrium / singular coupling.
  • Every @threads region runs on 1 thread in CI, so none of the "bit-identical across thread counts" behavior is exercised by automation.

The specific untested path: for a surface with (near-)integer q, the same surface index s appears on multiple rows (n=1 → m, n=2 → 2m, …). This repeated-s path is only reachable in a multi-n PE run. A write-only data race on sing[s].grri/.grre lived exactly here (removed in #325 424463c); any future per-(s,n) singular-surface state would reintroduce this hazard with no regression coverage to catch it.

Ask

Add a multi-n perturbed-equilibrium regression case whose equilibrium + n-range place an integer-q rational surface in range at more than one n, and verify the threaded coupling output is deterministic vs serial and across thread counts.

Subtleties to work through (why this warrants its own PR)

  1. Case selection. Need an equilibrium where an integer-q surface resonates at multiple n within both [nlow, nhigh] and [mlow, mhigh] — i.e. n·q lands on an in-range integer m for ≥2 values of n. This must be deliberately constructed, not assumed.
  2. New example deck. No existing example runs multi-n PE. Likely need a new examples/…_multi_n_pe_example/gpec.toml (Solovev-based to keep runtime low) with [PerturbedEquilibrium] enabled and [ForcingTerms] forcing spanning the n-range. Follow the TOML annotation conventions in CLAUDE.md.
  3. Quantity extraction. The coupling outputs (C_resonant_*, resonant_*, island half-widths, delta_prime, b^r) are per-row (surface, n). The harness case needs to track them as arrays with a stable, n-keyed ordering so diffs are meaningful.
  4. Thread-count determinism. The harness currently caches by commit, not by thread count. Decide how to assert bit-identity between serial and JULIA_NUM_THREADS>1 (e.g. a dedicated threads-vs-serial test outside the commit-cached harness, or a harness extension) and how BLAS pinning interacts.
  5. CI. Consider a threaded CI job (or a targeted threads-vs-serial unit test) so the @threads paths in SingularCoupling, FieldReconstruction, and KineticForces/Compute.jl are actually exercised — today they are not.
  6. Runtime budget. Multi-n PE is heavier; keep the case small enough for CI/harness time.

Context

Activity

  1. jhalpern30 commented on Jul 29, 2026

    @jhalpern30
    Collaborator

    @logan-nc assuming I am understanding this right, I think this is more than just adding a regression test - the multi-n code needs to be fully propagated into the perturbed equilibrium module. I think right now, there are some assumptions that n = nlow throughout the PE code.

    I think this will ultimately need to be a rather large PR from me as a step towards fully 3D PE, and in that PR I will add unit tests + a regression case for multi-n PE.

    Agree?

  2. jhalpern30 commented on Jul 29, 2026

    @jhalpern30
    Collaborator

    In one of my WIP branches, I think I actually worked out the current flags we need to block PE calculations - I think we currently only do the first one:

    1. if ctrl.force_termination = true (PE calculations not desired)
    2. if et[1] < 0 (plasma unstable)
    3. if wall_settings.nowall = false (stability calcs used a wall)
    4. if npert > 1 (multi-n not implemented yet)
  3. jhalpern30 commented on Sep 29, 2026

    @jhalpern30
    Collaborator

    Closed by #477. PerturbedEquilibrium (PE) now runs multi-n, and the new diiid_multi_n regression case covers the repeated-surface path this issue asked for:

    • The q = 2 and q = 3 surfaces resonate at both n = 1 and n = 2, so the same surface index appears on more than one row in SingularCoupling.
    • The case tracks every per-row quantity (resonant field, island half-width, Chirikov parameter, Δ′), with rows sorted by ψ and labelled by n and m.
    • It runs in about 150 s.

    Checking that threaded results match serial, and running CI with more than one thread, are not covered. They move to #479, along with the two missing PE guards.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    featureNew capabilitytestTests only

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions