Skip to content

Repo - API! - Time stages in main and drop the runtimes keyword - #552

Merged
matt-pharr merged 3 commits into
test/deck-api-parityfrom
refactor/stage-timing-in-main
Oct 26, 2026
Merged

matt-pharr merged 3 commits into
test/deck-api-parityfrom
refactor/stage-timing-in-main

Conversation

@matt-pharr

@matt-pharr matt-pharr commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Release note

  • Audience: users
  • Numerical impact: none (harness @ 4d75b80)
  • Migration: perturbed_equilibrium(...) no longer accepts a runtimes keyword; drop it. gpec.h5 no longer has Info/Runtimes/galerkin or Info/Runtimes/forcing_terms; the Galerkin and forcing times are part of force_free_states and perturbed_equilibrium.

Stage timing now lives only in main: the public perturbed_equilibrium and the other stage functions take no runtimes collector, and each stage's banner and wall-clock time are printed and recorded once.

Regression report

Stacked on #546, so the baseline is test/deck-api-parity.

SDCC fan-out at 4d75b8047 (vs test/deck-api-parity @ 0cd823948) is running after the rebase onto develop 2842dfc2d;
the full report replaces this block when it lands.

Notes for reviewers

Stacked on #546 (stack B); review that first. Closes blocker-4 ("no runtimes= keyword threaded into stages") and the stage-timing-banners duplication.

  • runtimes is removed from run_force_free_states, run_perturbed_equilibrium, the public perturbed_equilibrium, PerturbedEquilibrium.compute_perturbed_equilibrium and run_slayer_stage.
  • main times each stage from outside. PerturbedEquilibrium, KineticForces and ErrorFields run through a private _timed_stage(f, runtimes, key, label) that prints the section banner, logs "completed in" and records the time. The banners and timers those three stage functions printed on top of main's own are deleted, so each stage is timed once. A deck without a [PerturbedEquilibrium] section no longer prints an empty "Perturbed Equilibrium completed in 0.000 s".
  • Tearing is timed in main around run_slayer_stage and recorded only when SLAYER returns a result. Its banner stays inside the function, because only the function knows whether [SLAYER] is enabled.
  • Info/Runtimes/galerkin and Info/Runtimes/forcing_terms are dropped, along with their annotations. Both were measured inside a stage, which a caller outside the stage cannot time. The Galerkin solve still logs its own time. Rerun from gpec.h5 skips Info/Runtimes, so old files are unaffected.
  • The force_termination early exit, a copy of the SLAYER and runtimes tail, now shares that tail through a run_response gate. SLAYER still writes to the ForceFreeStates file, and the return tuple is unchanged.
  • The equilibrium and ForceFreeStates blocks of main already timed themselves once and are untouched apart from the dropped keyword.

Overlaps: #549 edits the same perturbed_equilibrium signature and docstring, and #547 rewrites the body of run_slayer_stage. Whichever lands second resolves a small textual conflict.

Tests: runtests_error_fields.jl now asserts that Info/Runtimes/forcing_terms is absent. runtests_solve_api, runtests_h5_schema, runtests_fullruns and runtests_error_fields pass locally.

🤖 Generated with Claude Code

@matt-pharr matt-pharr self-assigned this Oct 21, 2026
@matt-pharr
matt-pharr requested a review from logan-nc October 21, 2026 10:10
@github-actions github-actions Bot added api Config key, output dataset or exported name changed changed-results Results move or an interface breaks - read before upgrading labels Oct 21, 2026
@matt-pharr
matt-pharr added this pull request to stack #553 October 21, 2026 10:51

@logan-nc logan-nc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Read in full. Ready to approve from my read.

Pushed f2be9f0 (tests only, stamp unaffected): the body says runtests_error_fields.jl asserts Info/Runtimes/forcing_terms is absent, but the diff only dropped the haskey; it now asserts !haskey for forcing_terms and galerkin.

Checked the force_termination path: the shared tail writes the same file (pe_file falls back to ctrl.HDF5_filename, matching the old nothing → result.control.HDF5_filename) and records total the same way, so the early-return deletion is behaviour-preserving. No stale Runtimes/galerkin or forcing_terms references anywhere on the branch (src, docs, harness cases).

@logan-nc

Copy link
Copy Markdown
Collaborator

Heads-up: GitHub did not start the pull-request workflows on f2be9f0 (my push above); no check runs exist for it after an hour although Actions itself is fine. A close/reopen or your next push will re-fire them. The change is non-src, so the harness stamp is unaffected.

@logan-nc logan-nc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at f2be9f0 (includes my test-only push asserting the dropped Info/Runtimes datasets are absent). CI did not start on that push; a close/reopen or your next push will re-fire it.

matt-pharr and others added 3 commits October 26, 2026 09:46
Remove the runtimes keyword from run_force_free_states, perturbed_equilibrium,
compute_perturbed_equilibrium, run_perturbed_equilibrium and run_slayer_stage.
main times every stage from outside: PerturbedEquilibrium, KineticForces and
ErrorFields each get one banner and one timer through _timed_stage, so they are
no longer timed twice. Info/Runtimes drops the galerkin and forcing_terms
sub-stage entries, which only a stage could measure from inside.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@matt-pharr
matt-pharr force-pushed the refactor/stage-timing-in-main branch from f2be9f0 to 4d75b80 Compare October 26, 2026 08:48
@matt-pharr
matt-pharr merged commit 578376d into develop Oct 26, 2026
10 of 17 checks passed
@matt-pharr
matt-pharr deleted the refactor/stage-timing-in-main branch October 26, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Config key, output dataset or exported name changed changed-results Results move or an interface breaks - read before upgrading

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants