Repository navigation
Repo - API! - Delete the no-op KineticForces and PerturbedEquilibrium keys and stop runs that set deprecated keys - #569
Conversation
…m keys and stop runs that set deprecated keys
github-actions
Bot
added
api
… into refactor/deprecate-noop-control-keys
… into refactor/deprecate-noop-control-keys # Conflicts: # docs/development/hdf5-conventions.md # test/runtests_kinetic.jl
logan-nc
left a comment
logan-nc
left a comment
There was a problem hiding this comment.
Read in full; every deleted key grepped on develop (none is read anywhere; filter_modes had one test-only use the PR replaces). Four things for you before I approve; nothing pushed.
- The replay stop has a large blast radius. Stopping a replay on retired keys is deliberate per the new
runtests_rerun_from_h5testset, so this is about scope and message, not intent:main(["run.h5"])runs the same stops on the deck stored inInput/gpec_toml_raw, and all eight example decks on develop setfixed_boundary/output_eigenmodes(somemoment), so every gpec.h5 written to date now refuses to replay, and the message tells the user to edit a gpec.toml a replay does not have (apply_toml_overrides!can set keys, not delete them). Either keep warn-and-drop on the replay path only, which the deleted test comment said was its purpose, or give the replay its own message ("re-run from the original deck"). The body mentions only gpec.toml. - Undisclosed removals.
KineticForcesState/MethodResultlosetphi,tsurf,teq,wtw,records,completed; theEnergyIntegrals/*HDF5 group andwrite_integration_records!go;PerturbedEquilibriumState.singular_coupling_metricsgoes. The release note names onlyprint_summaryandEnergyIntegrationResult. TheEnergyIntegralsgroup is a schema removal and belongs in the Migration line even if nothing wrote it in practice. - No end-to-end test of the stop.
runtests_equil.jlcalls_stop_on_deprecated_keysdirectly and the onlymainpath tested is theuse_parallelrerun. One@test_throwson a staged deck with[KineticForces] moment = 1would cover the two new call sites. - Message grammar for a single key ("sets
moment, which no longer exist; remove them"). Suggest:"[$section] sets $(join(...)) — no longer a key; remove from gpec.toml", or pluralise onlength(found).
Docs hunks (kinetic_forces.md, hdf5-conventions.md, toml-conventions.md) match the code; the pre-commit regex extension is the right companion.
Collision facts, measured with git merge-tree: 2 hunks vs develop (hdf5-conventions.md, PerturbedEquilibrium.jl); 12 hunks vs the KineticForces stack #579→#584 (the ximag/dlambda/divxfac deletions vs the signatures that stack rewrote) and Torque.jl vs #588. Whichever side lands second takes the rebase; on that stack it is four harness re-stamps, here it is one.
…-noop-control-keys
…it to the original deck A replay checks its stored deck before overrides and says the file was written by an older GPEC version, since overrides cannot delete keys. The stop message pluralizes on the number of keys found. Tests cover the stop through main on a staged deck and on a replayed gpec.h5.
07e137e to
05d1faa
Compare
|
Thanks, all four are addressed in 05d1faa. Taking them in order:
Restamped against develop (578376d) over all 19 cases: nothing moves (263 quantities unchanged, |
…-noop-control-keys
…-noop-control-keys
logan-nc
left a comment
logan-nc
left a comment
There was a problem hiding this comment.
Re-read 05d1faa and the develop merge on top. All four items are in: the replay check runs every list on the stored deck before overrides and stops with the "older GPEC version" wording; the Migration line names every removed field, function and the EnergyIntegrals/ group; main on a staged deck with [KineticForces] moment = 1 and a replay with a stored fixed_boundary are both tested end to end; the message is worded for one key or several. Ready from my side.
The only red is the stamp check: src/ changed after 05d1faa through the develop merge (1ce3f7b), so CI wants the harness re-run at the current head and the stamp updated. A three-way merge against develop shows no conflicting hunks, so the DIRTY badge should clear on its own.
…-noop-control-keys
… re-formed analytic grid to a passing test setup_equilibrium is gone from develop; three tests still called it. The equilibrium now stores its analytic config, so the deck/API parity test's known drift no longer occurs.
Release note
[KineticForces]:moment,eq_out,theta_out,xlmda_out,eqpsi_out,save_records,gpec_file,fnml_flag,ellip_flag,diag_flag,force_xialpha.[PerturbedEquilibrium]:fixed_boundary,output_eigenmodes,filter_modes,singular_point_method. The keys already on the[ForceFreeStates]and[Equilibrium]lists now stop the run too, where they used to be dropped with a warning. Replaying a gpec.h5 whose stored deck sets any of them stops as well, saying the file was written by an older GPEC version; re-run from the original deck instead. In scripts,KineticForcesControlandPerturbedEquilibriumControl(and soperturbed_equilibrium) no longer accept those keywords;integrate_energy,evaluate_energy_integrandand the pitch integrators no longer acceptximag;compute_kinetic_matrices_at_psi!no longer takes adivxfacargument;KineticForces.print_summary,EnergyIntegrationResultandwrite_integration_records!are gone; and these fields are removed:KineticForcesInternal.tphi,tsurf,teqandwtw,MethodResult.records,KineticForcesState.completed,BounceData.dlambda,EnergyParams.ximag,PitchGARParams.ximagandPerturbedEquilibriumInternal.singular_coupling_metrics. TheEnergyIntegrals/group under each KineticForces method leaves the gpec.h5 schema; no run wrote it, since its writer never had records to write.Fifteen
[KineticForces]and[PerturbedEquilibrium]keys were parsed into control fields that nothing read, so setting them changed nothing. They are deleted, and a deck that still sets one, or any other key removed earlier, now stops and says which key to remove instead of running on without it.Regression report
One SDCC fan-out over all 20 harness cases at 8 CPUs per task (Julia 1.12.6 with the CI manifest pin) tracked develop (db90e82), #569 (7fd923d) and #574 (125f4a0) in a single run. Checked against that run's results database rather than only the report's last-vs-previous summary, every stored value other than runtime is bit-identical from develop to #569 and from #569 to #574: 271 tracked values in 19 cases per step, integers, reals, full arrays and checksums alike.
gal_resistive_pereads its 8 expected N/A on all three refs, since develop refuses that Galerkin PE run too.The stamp is 5786d2d, which adds only a test commit on top of 7fd923d: three tests call
PlasmaEquilibriumnow that #548 removedsetup_equilibrium, and the deck/API parity check that #548 fixed is promoted from@test_broken.Full report (tracking all three refs)
Notes for reviewers
Base. #556 has merged, so this PR now targets develop and the report compares against it. #574 is stacked on this branch.
What a listed key does now.
_drop_deprecated_keys!warned, deleted the key and let the run continue. It is now_stop_on_deprecated_keys, which raises one error naming every listed key the section sets, worded for one key or several, and asking the user to remove them from gpec.toml. All five call sites stay:[Equilibrium],[ForceFreeStates],[KineticForces]and[PerturbedEquilibrium]in the driver, and[Equilibrium]inRerun.jl. A replay also checks its stored deck before applying overrides, which can set keys but not delete them, and stops with its own message: the file was written by an older GPEC version, so re-run from the original deck. The stop is deliberate there too, not a warn-and-drop, so a replay never silently runs a different deck than the one recorded. Thetoml-no-deprecated-keyspre-commit hook covers the 15 new keys.test/runtests_equil.jlchecks that each list stops with the right wording, that a clean table passes, thatmainstops on a staged deck with[KineticForces] moment = 1, and that no listed[ForceFreeStates],[KineticForces]or[PerturbedEquilibrium]key is still a control field; thepower_*keys are still internalEquilibriumConfigfields derived fromjac_type, so that check skips[Equilibrium].test/runtests_rerun_from_h5.jlreplays a gpec.h5 whose stored deck setsfixed_boundaryand checks the replay message. #568 putlocal_stability_flagon the[ForceFreeStates]list, so with this PR a deck that still sets it stops too, and the hook covers it. (#554 settledgal_cut_solution: it is now aGalerkin(; cut_solution=true)solve option, not a deck key.)Dead code removed.
KineticForcesInternal.tphi/tsurf/teq/wtw,KineticForcesState.completed,BounceData.dlambdaand the weights computed only for it, theiunitconstant, andprint_summary.EnergyIntegrationResultwas never constructed, soMethodResult.recordswas always empty andwrite_integration_records!never ran. The struct, the field, the writer, its tenEnergyIntegrals/schema annotations, and the docs describing that group (kinetic_forces.md,hdf5-conventions.md) are gone.ximag, accepted for backward compatibility and never read: the field onEnergyParamsandPitchGARParams, and its keyword in the energy and pitch integrators. The unuseddivxfacargument ofcompute_kinetic_matrices_at_psi!goes too.PerturbedEquilibriumInternal.singular_coupling_metricsand an unusedreaddlmimport.PerturbedEquilibrium.jlandResponseMatrices.jl, and the issue references in PerturbedEquilibrium and KineticForces source. Thepower_*docstrings now say "not a deck key".fixed_boundaryin the Fortran benchmark script's generated decks.fixed_boundaryhad a Fortran meaning. Fortran'sfixed_boundary_flagsets the control-surface flux to the applied flux (foutmn = finmninstead ofpermeabmats · finmn) while the interior still answers it; only the a10 and a5 benchmark examples (the MARS/PENT a10 NTV benchmark) set it true. Julia once carried the same branch in singular coupling, and it was lost when that loop was restructured per toroidal mode in February. Nothing has read the key since, so it is deleted here; restoring it is a small change if the a10 PE benchmark is ported.Unchanged on purpose.
write_outputs_to_HDF5andoutput_filenamestay for the save-function work;xs_m,dbob_manddivx_mare live.🤖 Generated with Claude Code