Skip to content

Count field evaluations by derivative depth, and on the magfie path - #510

Open
krystophny wants to merge 1 commit into
mainfrom
feat/integrator-cost-instrumentation
Open

Count field evaluations by derivative depth, and on the magfie path#510
krystophny wants to merge 1 commit into
mainfrom
feat/integrator-cost-instrumentation

Conversation

@krystophny

Copy link
Copy Markdown
Member

Groundwork for the symplectic-vs-explicit integrator benchmark. Two things made "accuracy per field evaluation" impossible to state honestly.

The magfie path was invisible

velo_can (the drift-kinetic RHS) calls magfie directly rather than going through field_can, so its evaluations were never counted at all. A short integmode=0 run reports 120670 field evaluations with this change and would have reported none of them before — so every previous cost comparison between the RK and symplectic paths was counting only one side.

The two families don't use the same kind of evaluation

Explicit Runge-Kutta needs first derivatives only (mode_secders=0); the implicit symplectic schemes need second derivatives (mode_secders=2) for their analytic Jacobians. Those cost measurably different amounts, so a single total conflates them. On the same short run:

total first-deriv second-deriv
integmode=0 (RK) 120670 120670 0
integmode=3 (symplectic) 179998 16 179982

The split is essentially disjoint, which is exactly why weighting the two separately is necessary rather than cosmetic.

Change

  • n_field_evaluations_d1 / n_field_evaluations_d2 in field_can_base, both threadprivate like the existing total.
  • Every backend routes through a new count_field_evaluation(mode_secders) instead of bumping n_field_evaluations itself, so the split cannot drift out of step with the total.
  • velo_can counts its magfie call (first derivatives, so d1).
  • write_output reports the split alongside the total.

Testing

test_field_eval_counters checks the counters against a hand-computed call sequence, not a recorded number, and checks both the sum and which side each evaluation lands on — a test that only checked the sum would pass just as happily with the split wired up backwards. It also drives the real Boozer backend and asserts a known number of eval_field calls moves the counters by exactly that number.

Also restores bench_secders_cost.x, the measurement that turned up the libneo first-derivative spline path being slower than the second-derivative one (itpplasma/libneo#408). Built but deliberately not registered with ctest, since a timing result is hardware-dependent and must not gate CI.

Verification

  • All 31 unit tests pass, including test_sympl_testfield and test_orbit_refcoords_rk45.
  • Golden records compare .dat files only, so the added stdout lines cannot affect them.

Note on scope: an earlier survey suggested field_can_flux and field_can_meiss were missing their counter increments. That was read off a stale branch — on current main both already increment, and this PR does not need to add them.

The integrator benchmark compares accuracy per field evaluation between the
explicit Runge-Kutta paths and the implicit symplectic ones. Two things made
that comparison impossible to state honestly.

The drift-kinetic right-hand side velo_can calls magfie directly rather than
going through field_can, so its evaluations were never counted at all. A short
integmode=0 run reports 120670 field evaluations with this change and would
have reported none of them before, which means every previous cost comparison
between the RK and symplectic paths was counting only one side.

The two families also do not use the same kind of evaluation. Explicit
Runge-Kutta needs first derivatives only (mode_secders=0); the implicit
symplectic schemes need second derivatives (mode_secders=2) for their analytic
Jacobians. Those cost measurably different amounts, so a single total conflates
them. On the same short run:

    integmode=0 (RK)          120670 total,  120670 d1,       0 d2
    integmode=3 (symplectic)  179998 total,      16 d1,  179982 d2

The split is essentially disjoint, which is exactly why weighting the two
separately is necessary rather than cosmetic.

Every backend now routes through count_field_evaluation instead of bumping
n_field_evaluations itself, so the split cannot drift out of step with the
total.

test_field_eval_counters checks the counters against a hand-computed call
sequence rather than a recorded number, and checks both the sum and which side
each evaluation lands on: a test that only checked the sum would pass with the
split wired up backwards.

Also restores bench_secders_cost.x, the measurement that found the libneo
first-derivative spline path was slower than the second-derivative one. Built
but deliberately not registered with ctest, since a timing result is
hardware-dependent and must not gate CI.

All 31 unit tests pass.

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review verdict: Approve

Summary

PR #510 splits the global field-evaluation counter (n_field_evaluations) into first-derivative (d1, mode_secders=0) and second-derivative (d2, mode_secders>0) sub-counters so that cost-per-accuracy comparisons between explicit Runge-Kutta (first derivatives) and implicit symplectic (second-derivative Jacobians) integrators can be done fairly. A single count_field_evaluation(mode_secders) subroutine now routes every backend (albert, boozer, flux, meiss, spectre, test) plus the previously-uncounted magfie drift-kinetic path, keeping the total and split in sync. OpenMP reductions in write_output were extended to sum all three counters. A unit test (registered) and a timing benchmark (deliberately unregistered) were added.

Findings

  1. [minor] test/tests/CMakeLists.txt:355TIMEOUT 300 is excessive for this test. The arithmetic check runs in microseconds; the backend check either runs quickly or skips gracefully when wout.nc is absent. A 60–120s timeout would be more appropriate and avoids tying up a CI slot if init_field ever hangs on a bad path.

  2. [minor] src/sub_alpha_lifetime_can.f90:397 — The new count_field_evaluation(0) on the magfie path is the correct fix (magfie supplies first derivatives only), but it increases the total n_field_evaluations relative to prior runs because this path was previously invisible. This is not a bug — it is a correctness improvement — but benchmark comparisons against old output will see a higher count. Worth a note in the PR description for anyone comparing before/after numbers.

Verdict

Approve — The design is sound: a single choke point keeps the total and split in lockstep, all six backends plus the magfie path are routed through it, OpenMP threadprivate and parallel reductions are correct, and the arithmetic test validates the split logic unconditionally while the backend test skips gracefully. No correctness or blocking issues found.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant