Repo - FEATURE - Per-stage runtime records in gpec.h5 under Info/Runtimes

[logan-nc]

Release note

• Audience: users
• Numerical impact: none (harness @ 592cfb5)
• Migration: none

Every GPEC run now records how long each stage took, under Info/Runtimes/ in the output file
(seconds, one dataset per stage that actually ran, plus total). The values nest rather than
partition the run — galerkin is measured inside force_free_states, forcing_terms inside
perturbed_equilibrium, and total spans everything —
so they do not sum. benchmarks/benchmark_git_branches.jl reports the per-stage deltas between two
refs. Timings are informational only: machine- and load-dependent, never regression quantities.

Regression report

Regression Report: diiid_n1
============================================================================================================
Ref 1: develop  @ 003e939e0 (2026-09-29)
       env: julia 1.11.7, x86_64-linux-gnu, manifest e0a28574 (pinned), 8 threads/8 BLAS
Ref 2: feature/h5-runtime-records  @ 592cfb5f3 (2026-10-01)
       env: julia 1.11.7, x86_64-linux-gnu, manifest e0a28574 (pinned), 8 threads/8 BLAS
------------------------------------------------------------------------------------------------------------
Quantity                                      develop          feature/h5-runtime-records  Diff       Status
------------------------------------------------------------------------------------------------------------
total energy Re(et[1])                        8.012318e-01     8.012318e-01                0.0e+00    OK    
total energy Im(et[1])                        4.142529e-05     4.142529e-05                0.0e+00    OK    
plasma energy Re(ep[1])                       -1.348486e+00    -1.348486e+00               0.0e+00    OK    
vacuum energy Re(ev[1])                       2.149718e+00     2.149718e+00                0.0e+00    OK    
vacuum matrix min eigenvalue                  1.873976e-01     1.873976e-01                0.0e+00    OK    
plasma energy (all)                           [35 elem]        [35 elem]                   0.0e+00    OK    
vacuum energy (all)                           [35 elem]        [35 elem]                   0.0e+00    OK    
total energy (all)                            [35 elem]        [35 elem]                   0.0e+00    OK    
ODE steps (saved)                             2576             2576                        0.0e+00    OK    
ODE steps (total)                             4572             4572                        0.0e+00    OK    
q0                                            1.204212e+00     1.204212e+00                0.0e+00    OK    
q95                                           4.781723e+00     4.781723e+00                0.0e+00    OK    
beta_t                                        1.327024e-02     1.327024e-02                0.0e+00    OK    
beta_n                                        1.372511e+00     1.372511e+00                0.0e+00    OK    
internal inductance li1                       8.842392e-01     8.842392e-01                0.0e+00    OK    
internal inductance li2                       7.080847e-01     7.080847e-01                0.0e+00    OK    
internal inductance li3                       7.304433e-01     7.304433e-01                0.0e+00    OK    
poloidal beta betap1                          6.680744e-01     6.680744e-01                0.0e+00    OK    
poloidal beta betap2                          5.349834e-01     5.349834e-01                0.0e+00    OK    
poloidal beta betap3                          5.518761e-01     5.518761e-01                0.0e+00    OK    
# singular surfaces                           5                5                           0.0e+00    OK    
singular psi locations                        [5 elem]         [5 elem]                    0.0e+00    OK    
singular q values                             [5 elem]         [5 elem]                    0.0e+00    OK    
current beta betaj                            4.236478e-01     4.236478e-01                0.0e+00    OK    
plasma volume                                 1.829472e+01     1.829472e+01                0.0e+00    OK    
plasma current                                1.152130e+00     1.152130e+00                0.0e+00    OK    
mpert                                         35               35                          0.0e+00    OK    
npert                                         1                1                           0.0e+00    OK    
toroidal field bt0                            2.006573e+00     2.006573e+00                0.0e+00    OK    
wall field bwall                              3.880145e-01     3.880145e-01                0.0e+00    OK    
aspect ratio                                  2.845746e+00     2.845746e+00                0.0e+00    OK    
elongation kappa                              1.708350e+00     1.708350e+00                0.0e+00    OK    
q profile (checksum)                          0cd285cea88d...  0cd285cea88d...             identical  OK    
pressure profile (checksum)                   a1c48b266622...  a1c48b266622...             identical  OK    
Mercier D_I profile (checksum)                eeb06744e795...  eeb06744e795...             identical  OK    
resistive interchange D_R profile (checksum)  fa37296851f6...  fa37296851f6...             identical  OK    
ballooning Delta' profile (checksum)          6eb0ea075ccf...  6eb0ea075ccf...             identical  OK    
island half-widths                            [5 elem]         [5 elem]                    0.0e+00    OK    
Chirikov parameter                            [5 elem]         [5 elem]                    0.0e+00    OK    
||resonant area-weighted field||              5.189179e-04     5.189179e-04                0.0e+00    OK    
dominant-coupling singular values             [3 elem]         [3 elem]                    0.0e+00    OK    
|forcing overlap with dominant mode|          1.415683e-04     1.415683e-04                0.0e+00    OK    
|delta_nominal| of coil set 1                 7.055228e-05     7.055228e-05                0.0e+00    OK    
||ddelta/d(shift)|| over coil sets            1.246233e-04     1.246233e-04                0.0e+00    OK    
||ddelta/d(tilt)|| over coil sets             4.180643e-07     4.180643e-07                0.0e+00    OK    
PE plasma energy                              3.422677e+00     3.422677e+00                0.0e+00    OK    
PE vacuum energy                              3.174510e+00     3.174510e+00                0.0e+00    OK    
PE surface energy                             5.826684e+00     5.826684e+00                0.0e+00    OK    
PE toroidal torque                            5.087465e-02     5.087465e-02                0.0e+00    OK    
NTV torque FGAR [N·m]                         5.296762e-01     5.296762e-01                0.0e+00    OK    
NTV kinetic energy dW FGAR [J]                7.924971e-02     7.924971e-02                0.0e+00    OK    
Runtime (s)                                   291.4s           296.4s                                 --    
||forcing b~|| (root-area-weighted)           4.683328e-04     4.683328e-04                0.0e+00    OK    
resonant area-weighted field b^r              [5 elem]         [5 elem]                    0.0e+00    OK    
============================================================================================================
Summary: 53 unchanged

What and why

main_from_inputs timed every pipeline stage but only logged the durations, so an output file could not answer "where did the time go". This persists them under Info/Runtimes/<stage> (Float64 seconds, long_name + units="s" per the self-describing-HDF5 contract from #364), and teaches benchmarks/benchmark_git_branches.jl to report per-stage deltas averaged over its warm runs.

Stages, each recorded only when it actually runs: equilibrium, galerkin, force_free_states, forcing_terms, perturbed_equilibrium, kinetic_forces, error_fields, tearing, plus total.

The values nest rather than partition: galerkin is measured inside the force_free_states window, forcing_terms inside the perturbed_equilibrium window, and total spans the whole run, so the numbers do not sum. Their long_names say so, since that is the one thing a reader of the file cannot infer.

Timings are informational only — machine- and load-dependent. They are not regression quantities and should never be added to the harness.

Notes for review

• The completion banner at both exit paths now reuses total_dt, so the log line and the recorded value cannot disagree.
• _write_runtimes! guards on isfile (no-op when write_outputs_to_HDF5=false and nothing else wrote a file) and deletes an existing Info/Runtimes before writing, so reruns into an existing file stay idempotent. It targets the file this run actually produced: ctrl.HDF5_filename normally, or the SLAYER stage's pe_file when write_outputs_to_HDF5=false.
• Annotation reuses the existing Utilities.HDF5Annotations.annotate!; no new annotation path. test/runtests_h5_schema.jl enforces the metadata automatically — its walker fails if any Info/Runtimes/* dataset lacks long_name/units.
• The equilibrium-only early exit is untouched (no h5 exists there yet).
• In the benchmark tool the Info/Runtimes read is inside the warm-run loop, since each run overwrites gpec.h5. Refs predating the feature yield an empty dict and the table is skipped silently, so old-vs-new comparisons keep working.
• The stage token is tearing, not slayer: ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B) #364 renamed the slayer/ group to Tearing/ under the "name the physics, not the algorithm" rule, and the other tokens already mirror their groups. The solver is named in the long_name instead.

Verification

Check	Result
runtests_h5_schema.jl	pass
runtests_fullruns.jl	pass (+2 runtime assertions)
runtests_rerun_from_h5.jl	pass (Info/Runtimes exempted from the bit-for-bit replay comparison)
Regression harness diiid_n1	48 unchanged, 0 changed — this diff only appends wall-clock numbers after all computation
Benchmark dry-check	read, missing-group fallback, missing-file fallback, shared-key averaging, total-last ordering, silent skip on empty/disjoint sides, zero-baseline Δ% guard

---

⚠️ NO PR IS EVER MERGED WITHOUT A THIRD-PARTY HUMAN REVIEWER'S APPROVAL — NON-NEGOTIABLE. ⚠️

🤖 Generated with Claude Code

---

Update 2026-09-28: brought current with develop, plus the ErrorFields stage

The branch was 309 commits behind and CONFLICTING. Merged develop; four conflicts, resolved per
docs/development/git-workflow.md rather than taking a side:

• prepare_force_free_states! / galerkin_solve: develop renamed the coefficient-matrix binding
  ffit → mats. Applied this branch's instrumentation to the renamed symbol.
• main_from_inputs force_termination early return: combined this branch's runtime recording with
  develop's widened named tuple (coil_sensitivities, monte_carlo, locking_risk, efc_couplings).
• runtests_fullruns.jl: both sides appended independent assertions at the same point; kept both.

Develop added a stage while this sat: run_error_fields, between KineticForces and SLAYER. It is
both a pipeline stage and an HDF5 write site of its own, and this feature tracked neither. Two
consequences, one cause:

1. No error_fields entry in Info/Runtimes, though the stage already measures its own wall-clock.
2. written_h5 did not mirror the ErrorFields write gate. With ForceFreeStates and PerturbedEquilibrium
   writes off and no SLAYER section, an ErrorFields run produces a real output file that no tracked
   stage wrote — so written_h5 stayed nothing and the timings were dropped silently. With a
   distinct output_filename the stamp could also land in a file that was not the last one written.

Fixed with one block after the stage call, matching the existing PE/KF pattern. The regression test is
the sole-writer configuration — ErrorFields writing to its own ef_only.h5 with every other write off
— which is precisely the case that was broken, and which no existing fixture covered.

Also corrected for CI: the title (GPEC is not an Area — the checker says use Repo; and the TAG must
be the single word FEATURE), and the missing ## Release note block.

Verification at b87a66aa9

• Pkg.test on runtests_error_fields.jl, runtests_h5_schema.jl, runtests_fullruns.jl,
  runtests_rerun_from_h5.jl — all pass.
• Regression harness diiid_n1, develop @ 786a12154 vs this branch: 53 quantities, all unchanged
  (table above). Expected: the feature appends timings after all computation.

A design note for the reviewer

The written_h5 bookkeeping now carries 6 tracking points in main_from_inputs mirroring 5 write
sites that live in 4 different functions, none adjacent to their mirror. @priyanshlunia flagged this
when resolving the last conflict: "If a default flips or a write gate gains a condition, the mirror in
main_from_inputs must follow." That drift then happened, within five weeks, via ErrorFields. The
fourth mirror is the minimal correct fix for this PR; a more robust design is discussed separately and
is deliberately not in scope here.

---

Update 2026-10-01: ForcingTerms runtime, test trim

Info/Runtimes/forcing_terms. ForcingTerms is its own workflow box between ForceFreeStates and
PerturbedEquilibrium, and on coil decks it can be a large share of the PE window:
compute_coil_forcing_modes! runs Biot-Savart from every coil segment onto the
mtheta_coil × nzeta_coil plasma-surface grid, once per toroidal mode. That cost was folded silently
into perturbed_equilibrium. It is now timed around the materialize_forcing_modes prologue and
recorded nested inside perturbed_equilibrium, exactly as galerkin nests inside
force_free_states. The collector threads run_perturbed_equilibrium → perturbed_equilibrium →
compute_perturbed_equilibrium as a defaulted keyword; perturbed_equilibrium takes it as a named
keyword so it is not swallowed into its PerturbedEquilibriumControl splat.

It is recorded only when materialization actually runs. Coil decks always record it. Runs with
injected modes — gpec.h5 replay, and ascii/hdf5 forcing decks, whose modes snapshot_forcing_modes
pre-loads in the orchestrator — short-circuit the prologue and carry no forcing_terms record.

Promoting ForcingTerms to a sibling section with its own banner is the structurally cleaner end state,
but it moves materialization out of the perturbed_equilibrium public API and has to settle whether
coil modes enter the Input/ replay snapshot, so it is deliberately left for a follow-up.

Test trim. The ErrorFields sole-writer test is down to its one decisive assertion; it is
self-guarding, since if any other stage were the last writer the record would not be in ef_only.h5.

Found in passing, not addressed here (develop bug): that same short-circuit means TOML-path
ascii/hdf5 forcing decks skip convert_forcing_normalization! — snapshot_forcing_modes discards
the file's declared normalization tag. On Solovev_ideal_example PE's forcing_b, forcing_b_rootarea
and dominant_forcing_overlap come out 16.848× smaller than with materialization. Coil decks are
unaffected. To be tracked separately; nothing in this PR changes that behaviour.

Verification at 592cfb5f3

• Pkg.test on runtests_error_fields.jl, runtests_h5_schema.jl, runtests_fullruns.jl,
  runtests_rerun_from_h5.jl, runtests_solve_api.jl (the last because perturbed_equilibrium's
  signature changed) — all pass.
• Regression harness diiid_n1, develop @ 003e939e0 vs this branch: 53 quantities, all unchanged.

⚠️ STILL REQUIRES A THIRD-PARTY HUMAN REVIEWER'S APPROVAL BEFORE MERGE — NON-NEGOTIABLE ⚠️

This PR has zero submitted reviews. No auto-merge enabled.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PSrf6JCViFfVzqzkQ66o6b

[logan-nc]

Verification complete — post-merge

All checks re-run against the merged tree (eb944f9b):

Check	Result
runtests_h5_schema.jl	pass (14/14 + 6/6 group-name rule)
runtests_fullruns.jl	19/19 pass (17 before; +2 new runtime assertions)
Regression harness, diiid_n1	48 unchanged, 0 changed

Regression table

Regression Report: diiid_n1
Ref 1: 0ece9c4f  @ 0ece9c4f (2026-08-12)   <- branch parent
Ref 2: e695e890  @ e695e890 (2026-08-14)   <- feature commits
Summary: 48 unchanged

Zero movement, as expected — this change computes time() differences and appends to the h5 after all computation.

Note for anyone rerunning the harness on this branch

Two baseline traps produced misleading reports along the way; both are documented in the handoff doc:

1. Don't compare against origin/refactor/hdf5-metadata's tip. This branch's parent was ~60 commits behind it, so that comparison reports upstream physics changes (auto psi-grid Δ′ convergence fix, on-demand solution derivatives, coil psilim fix) — 38 quantities, nothing to do with this PR.
2. Don't compare a commit ref against local. Non-local refs run in a freshly instantiated harness worktree while local runs the working-tree environment, so the comparison includes environment drift. 0ece9c4f vs local shows 14 quantities flagged at 0.00% plus ODE steps (total) 1960 → 1968 — pure artifact. Comparing two commit refs through the same worktree path gives the clean zero above.

The case itself is deterministic: two --force runs of the same commit produced byte-identical values for all 49 quantities.

---

⚠️ STILL REQUIRES A THIRD-PARTY HUMAN REVIEWER'S APPROVAL BEFORE MERGE — NON-NEGOTIABLE. ⚠️

Also still required before merge: delete HANDOFF_h5_runtime_records.md.

[logan-nc]

Brought current with develop + four corrections

Branch updated. #364 merged this morning, so GitHub auto-retargeted this PR from refactor/hdf5-metadata to develop. Merged origin/develop in (641504fb) — content-free, since the merge base was already #364's head and develop's only new commit was that merge. The PR diff is now exactly this feature's 6 files, and with the base on develop the full Julia test suite runs in CI on this PR for the first time (test.yaml only triggers on PRs into main/develop, so it never ran while this was stacked).

Re-reviewed against the conventions as they finally landed. Four things needed fixing (40e550e3):

1. Info/Runtimes/slayer → tearing. ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B) #364 renamed the slayer/ group to Tearing/ under the "name the physics, not the algorithm" rule; every other stage token already mirrors its group, so this was the straggler. SLAYER is now named in the long_name, where the solver belongs.

2. perturbed_equilibrium was recorded unconditionally. The push! sat outside the if "PerturbedEquilibrium" in keys(inputs) block, so a stability-only run stamped the stage at ~0.000 s for work that never happened — contradicting this PR's own "recorded only when they actually run" claim, and reachable in the existing fullruns examples.

3. _write_runtimes! could stamp a file this run never wrote — or skip the one it did. It hardcoded ctrl.HDF5_filename, but the SLAYER stage appends to pe_file, and PerturbedEquilibrium and KineticForces each carry their own write_outputs_to_HDF5 flag and filename. A run with ctrl.write_outputs_to_HDF5=false and pe_ctrl.write_outputs_to_HDF5=true produced a real output file that the guard ignored entirely, silently losing the timings. Each of the four write sites now records its path in a written_h5 local (last writer wins, matching where SLAYER already appends), and both exit paths stamp that file. This also replaces the write_outputs_to_HDF5 || slayer_result !== nothing proxy with the fact it was approximating.

4. print_stage_comparison divided by the branch-1 time with no zero guard, printing Inf%/NaN%. Item 2 made that reachable today; the column now shows an em dash when the baseline is zero.

Also: the galerkin and total long_names now state that the values nest rather than partition the run — galerkin is measured inside the force_free_states window and total spans everything, so the seven numbers do not sum. That is the one thing a reader of the file cannot infer. And docs/src/workflow.md's Info/ row now mentions Runtimes/, matching the row this PR already updated in hdf5-conventions.md.

The rest reviewed clean: the annotation table, the Info/Runtimes exemption in the replay comparison, the inside-the-warm-loop placement of the benchmark read, and the empty-dict fallback for refs predating the feature.

Local runtests_h5_schema.jl / runtests_rerun_from_h5.jl / runtests_fullruns.jl are running against the final tree now; I will post the results and correct anything they turn up.

Important

This PR requires a third-party human review approval of its current head before merge. No auto-merge label applied.

[logan-nc]

All checks green, and the suite ran against this branch in CI for the first time now that the base is develop:

Check	Result
runtests 1.11 - ubuntu-latest	pass
runtests 1.x - ubuntu-latest	pass
Tests	pass
Documentation / documenter/deploy	pass

Local, against the final tree: runtests_h5_schema.jl, runtests_rerun_from_h5.jl, and runtests_fullruns.jl all pass (19/19 on fullruns, including the two runtime assertions).

Direct check of the written files on two real runs, confirming the two behavioural fixes:

DIIID-like_SLAYER_example          -> [equilibrium, force_free_states, tearing, total]
Solovev_kinetic_calculated_example -> [equilibrium, force_free_states, total]

The SLAYER run records tearing, not slayer. The example with no [PerturbedEquilibrium] section records no perturbed_equilibrium key — before the gating fix it recorded one at ~0.000 s. Neither example configures Galerkin or KineticForces, so those tokens are correctly absent as well.

Important

Green CI is not review. This PR still requires a third-party human approval of its current head (40e550e3) before merge. No auto-merge label applied.

[logan-nc]

@priyanshlunia review & merge this in the next 24 hrs please. Just fix any minor issues yourself directly. Ping me if anything significant changes.

[priyanshlunia]

Context. The PR head (40e550e3) instrumented the monolithic main_from_inputs:
every stage timer, every HDF5 write site, and the _run_slayer_stage closure lived in one
function, so runtimes and written_h5 were plain locals visible to everything. Develop
then landed the staged-pipeline refactor (#393 line): main_from_inputs became a thin
orchestrator calling run_force_free_states, run_perturbed_equilibrium,
run_kinetic_forces, and run_slayer_stage, with the PE write moving inside the public
perturbed_equilibrium API. Two of the four write sites and two of the timers moved into
functions the orchestrator's locals can't reach.

The resolution taken: keep all bookkeeping decisions in main_from_inputs
and infer what the stage functions did, rather than changing their return values.
Two stage functions get an optional collector kwarg for the timers they own; everything
else is timed and tracked at the call sites. Merge commit: 30529ff7.

The three mechanisms

1. Call-site timing (equilibrium, force_free_states, perturbed_equilibrium, kinetic_forces, total)

main_from_inputs wraps each stage call in a time() window and pushes the record itself.
Semantics match the PR head: perturbed_equilibrium is recorded only when the deck has a
[PerturbedEquilibrium] section (the stage function logs its window unconditionally, but a
section-less run must not stamp a ~0 s entry), and kinetic_forces only when
[KineticForces] is present.

2. Collector kwarg (galerkin, tearing)

Two timers measure something inside a stage function, so the call-site window can't
reproduce them:

• run_force_free_states(...; runtimes=nothing) — pushes "galerkin" => dt around the
  Galerkin solve. Note the semantic shift inherited from develop: galerkin is now an
  exclusive integrator choice, not a gal_flag add-on, but it still nests inside the
  force_free_states window, so the annotation wording holds.
• run_slayer_stage(...; runtimes=nothing) — pushes "tearing" => dt around
  Runner.run_slayer only (excluding the HDF5 append), preserving the PR head's
  solver-only semantics.

Both default to nothing and no-op when absent, so the scripting API path
(solve(::EulerLagrangeProblem, ...), which also calls run_force_free_states) is
completely untouched.

3. Write-site inference (written_h5)

The PR head assigned written_h5 directly at each of the four write sites. Two of those
sites are now out of reach, so the orchestrator mirrors their write gates instead
(src/GeneralizedPerturbedEquilibrium.jl:244–332):

Stage	Where the write happens now	Inference in main_from_inputs
ForceFreeStates	still in the orchestrator	direct assignment inside the existing if ctrl.write_outputs_to_HDF5 block (unchanged)
PerturbedEquilibrium	inside public perturbed_equilibrium, gated on pe_ctrl.write_outputs_to_HDF5 (default true) with filename fallback output_filename == "" → ctrl.HDF5_filename	section present && get(inputs["PerturbedEquilibrium"], "write_outputs_to_HDF5", true) → written_h5 = pe_file; the pe_file derivation (which already mirrored the filename fallback for SLAYER) was hoisted above the PE call
KineticForces	inside run_kinetic_forces, gated on section && pe_state !== nothing && kf_ctrl.write_outputs_to_HDF5	same three conditions, all visible in the orchestrator (kf_ctrl is in scope)
SLAYER	inside run_slayer_stage's guarded try	slayer_result !== nothing ⟹ the Tearing/ append succeeded → written_h5 = pe_file (or ctrl.HDF5_filename on the force_termination path)

"Last writer wins" ordering and the two stamp points (the force_termination early return
and the final banner) are unchanged from the PR head, as are _write_runtimes!,
RUNTIME_H5_ANNOTATIONS, the tests, the benchmark-script changes, and the docs rows.

What changed relative to the PR head, in one list

1. Removed the _run_slayer_stage closure instrumentation (the closure itself is gone
   on develop) → replaced by the runtimes kwarg on the top-level run_slayer_stage.
2. Moved the galerkin timer into run_force_free_states via the same kwarg pattern.
3. Moved PE/KF timing from inside the (former) inline blocks to call-site windows —
   numerically equivalent (same window + function-call overhead).
4. Replaced direct written_h5 assignments at the PE and KF write sites with the
   gate-mirroring inference above; SLAYER's assignment became result-based inference.
5. Reverted a bad auto-merge: git had spliced the PR's push!(runtimes, "perturbed_equilibrium" => ...) into run_perturbed_equilibrium, where runtimes
   doesn't exist — it parsed fine and would have thrown UndefVarError on every PE run.

Trade-off and review focus

This implementation keeps the diff minimal and every stage function's signature/return contract
intact (only two additive kwargs), at the cost of duplicating three write-gate
conditions in the orchestrator. If a default flips (e.g.
PerturbedEquilibriumControl.write_outputs_to_HDF5) or a write gate gains a condition, the
mirror in main_from_inputs must follow — each mirror carries a one-line comment naming
what it mirrors. These three conditions are the right review focus; the alternative
(stage functions return the filename they wrote) is drift-proof but touches
internal signatures — easy to switch to later if the mirrors prove fragile.

Verification: 99 tracked harness quantities across 5 runnable cases, zero moved;
19/19 fullruns tests plus schema and replay tests pass; Info/Runtimes confirmed in real
outputs including the tearing record.

[priyanshlunia]

@logan-nc pinging you since this was a relatively significant edit.

[logan-nc]

@priyanshlunia this will auto-merge if/when you change the review to "accept". Thanks