Skip to content

[math-01] Inventory formulas and lock KAMEL conventions#222

Draft
marjohma wants to merge 1 commit into
fix/kim-fp-37-parallel-flowfrom
fix/kim-fp-193-conventions
Draft

[math-01] Inventory formulas and lock KAMEL conventions#222
marjohma wants to merge 1 commit into
fix/kim-fp-37-parallel-flowfrom
fix/kim-fp-193-conventions

Conversation

@marjohma

Copy link
Copy Markdown
Member

Summary

  • add a formula index mapping thesis equations to code consumers
  • lock phase, Fourier, units, species, and sign conventions in executable Mathematica checks
  • add shared deterministic verification helpers and Makefile wiring

Why

Later physics changes have an explicit convention baseline and traceable source map.

Validation

Validated cumulatively at the stack head:

  • cmake --build build -j2
  • ctest --test-dir build -E 'test_rhs_balance|test_periodic_convergence|test_periodic_vs_global' --output-on-failure — 35/35 passed

Known red gates are intentionally excluded from that command: the pre-existing test_rhs_balance baseline failure, the periodization-deformation acceptance gate, and the unresolved periodic-vs-global 5% agreement gate.

Stack

Base: fix/kim-fp-37-parallel-flow
Head: fix/kim-fp-193-conventions

Closes #193

@marjohma

Copy link
Copy Markdown
Member Author

Review (standards + spec vs #193) — keeping this in draft. The structure (index + checklib + Makefile targets) is delivered and several spot-checked convention rows match the code (vT vs species_mod.f90:477, qres=abs(m/n) vs prepare_resonances.f90:24, wavevectors vs calculate_equil.f90:160-162; checklib's Exit[1]-on-failure semantics are sound). But the ledger's core claims don't hold up yet:

Blocking

  1. Phantom code citations in the source-of-truth index: FORMULA_INDEX.md:15 cites KIM/src/general/constants.f90 (doesn't exist — actual: KIM/src/util/constants_mod.f90); FORMULA_INDEX.md:21 cites module fourier_grid_m (exists nowhere on the branch). Both escape the script's "all indexed code paths exist" check because it only covers the hand-listed 22-entry codePaths. A ledger meant to lock conventions must itself be machine-checked.
  2. "Automated counts" are hardcoded: chapterMaxima and the 494/24/19/8 literals in 01_conventions_and_inventory.wl:10-16,48-53,92 are hand-entered; the thesis side is asserted (PDF referenced by MD5 only, not present), and the "Fortran formula sites" count counts Markdown rows, not code sites. Spec: "Automated counts for thesis displayed equations, indexed equations, Fortran formula sites, and classified sites."
  3. Negative fixture "missing code formula → exits nonzero" is not implemented: nothing scans the Fortran, so a physics formula absent from the index is undetectable; duplicate convention rows in the Markdown also escape (no duplicate check on conventionRows).
  4. verify-mathematica-inventory-negative can false-pass: the inner make's output is discarded (>/dev/null 2>&1), so on a machine without WolframKernel the "kernel not found → exit 1" path is indistinguishable from a genuine fixture rejection — the target exits 0 claiming all four fixtures rejected.
  5. No evidence the scripts have ever been executed: no CI step (.github/workflows/ runs cmake/ctest only), no ctest registration, no committed run log; the PR's validation section covers only the Fortran build. Reviewer has no Mathematica available either — please attach a run log or wire a CI job.

Should be fixed too

  • Only 8 of the 19 convention rows are executably locked via registerConvention; coordinate/field orientation, time dependence, velocity-space measure, Bessel/plasma-dispersion definitions, collision normalization, and boundary/gauge exist as prose only ("Lock … every …" is the issue's core demand).
  • The "identity checks" (Exp[I kr r] Exp[-I kr r]==1, conjugate reality, CGS dimension arithmetic; script lines 96-121) are tautologies — true under any convention, so they lock nothing against the code.
  • Thesis-table "Consumers" are chapter-blanket ("EQ/KIM/KiLCA/QLB" for every ch.2 row) rather than the "all consuming code symbols" the issue asks for.
  • KILCA-04 classifies a PreProc path under the KiLCA prefix, contradicting the file's own legend.
  • Minor: commit type verify: is outside the documented set in AGENTS.md (feat/fix/refactor/chore); registerConvention source strings use "C-vT"/"C-radial-FT" vs index IDs C-vt/C-radial-ft.

@marjohma marjohma left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review: standards + spec

Reviewed 9222ece5 against the PR base fix/kim-fp-37-parallel-flow (stacked PR — not main), on two axes: conformance to the repo's documented standards (AGENTS.md, CONTRIBUTING.md) and fidelity to #193.

The harness itself is sound. 12 checks, exit 0 on the baseline, and all four negative fixtures correctly exit 1 — FinishChecks[] really does set a nonzero exit code. The stated non-goal is respected: the ledger says “Indexed… does not mean independently verified” (FORMULA_INDEX.md:8-9) and no row claims otherwise. The index itself is a substantial, genuinely useful artifact.

The blocking concern is that the physics checks are tautologies. All 12 checks operate on literals typed into 01_conventions_and_inventory.wl itself; none reads a .f90 body or the thesis. Both Fourier checks pass under either sign convention, so they lock nothing — which is exactly what #193 was written to prevent: “No later issue may silently introduce a different sign, phase, velocity normalization, unit system, coordinate orientation, or Fourier measure.” A green gate whose greenness is independent of the code under test is worse than no gate, because downstream it reads as coverage. Details inline.

Spec gaps (#193)

  • Automated counts absent. Spec asks for “automated counts for thesis displayed equations, indexed equations, Fortran formula sites, and classified sites”. Only indexed and classified rows are counted, both from the same markdown. The thesis PDF is referenced by MD5 (FORMULA_INDEX.md:7) but never opened; no .f90 is ever read (only FileExistsQ).
  • 11 of the 19 ledger conventions have no check. Only 8 reach registerConvention. Unregistered: C-reality, C-coords, C-field, C-wavevectors, C-flow, C-bessel, C-pdf, C-collision, C-velocity-measure, C-boundary, C-parity. #193 explicitly names coordinate and field orientations, velocity-space measure, Bessel, plasma-dispersion, collision normalization and boundary/gauge among those to lock.
  • Gyro/Larmor signs unchecked. Spec says “gyrofrequency and Larmor-radius signs”; only inverse-time units are asserted, and Larmor radius is absent from the script entirely.

Scope creep

Minor and defensible: verify-mathematica-inventory-negative goes beyond the single target #193 asks for. It earns its keep — it's what proves the negative fixtures actually fail.

Standards

Two hard violations:

  1. Commit type verify is not an allowed type. 9222ece5 is verify(KAMEL): lock formula conventions. AGENTS.md: “Commit style: Conventional Commits (feat:, fix:, refactor:, chore:)”; verify is in neither that list nor the wider conventional set. Scope KAMEL is also the whole repo, where every other scope is a subproject (feat(KIM): add new solver is the documented example). test(KIM): or docs(KIM): would comply.
  2. Lines over 100 columnsAGENTS.md → Coding Style: “Line width: 100 columns maximum”. Flagged inline.

Judgement calls:

  1. verification/ isn't in the docs. AGENTS.md keeps an explicit “Project Structure” list and a make-target list; neither gains verification/ nor the two new targets. The direct precedent, test/golden/, is documented in AGENTS.md and carries its own README. verification/ has none.
  2. IndentationAGENTS.md says “Indentation: 4 spaces (no tabs)”; both .wl files use 2. No .wl precedent exists in-repo, so the rule likely wasn't written with Wolfram in mind. Noting, not pressing.

.PHONY is correctly declared for both new targets. English-only, inclusive terminology and comment discipline are all clean. Pre-commit-enforced items not re-checked.

Comment on lines +99 to +106
phase[mm_, nn_] := Exp[I (mm theta + nn phi)];
check[
"Fourier conjugate reality",
TrueQ[Simplify[
phase[-mm, -nn] == Conjugate[phase[mm, nn]],
Element[{mm, nn, theta, phi}, Reals]
]]
];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This check is vacuous — it passes under either sign convention.

Conjugate[Exp[i x]] == Exp[-i x] is an identity of Exp for real arguments. Flip line 99 to Exp[-I (mm theta + nn phi)] and the check still returns True, because phase[-mm,-nn] and Conjugate[phase[mm,nn]] flip together. It therefore asserts nothing about the exp(+i(m theta + n phi)) convention #193 asks to lock, and nothing about KAMEL's Fortran.

phase[1, 0] === Exp[I theta] is sign-sensitive and would at least pin the registered definition. Better still, derive the assertion from the harmonic assembly the code actually implements (kernel.f90, fouriermodes.f90) so the check fails when the code drifts — which is the whole point of the gate.

Comment on lines +107 to +110
check[
"radial Fourier signs are inverse pairs",
Simplify[Exp[I kr r] Exp[-I kr r], Element[{kr, r}, Reals]] === 1
];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also a tautology. Exp[I kr r] Exp[-I kr r] is Exp[0] — identically 1 for any signs, so it cannot distinguish the forward transform from the inverse.

C-radial-ft (FORMULA_INDEX.md:21) locks “inverse exp(+i k_r r)/(2 pi); the forward transform uses exp(-i k_r r). Nothing here tests that assignment, nor where the 1/(2 pi) measure lands. A round-trip on a concrete function — forward then inverse, asserting both identity recovery and measure placement — would be sign- and measure-sensitive.

Comment on lines +48 to +72
codePaths = {
"KIM/src/background_equilibrium/species_mod.f90",
"KIM/src/background_equilibrium/calculate_equil.f90",
"KIM/src/asymptotics/flr2_fourier_kernel.f90",
"KIM/src/kernels/FP_kernel_plasma_prefacs.f90",
"KIM/src/kernels/kernel.f90",
"KIM/src/electrostatic_poisson/poisson.f90",
"KIM/src/electrostatic_poisson/periodic_solve.f90",
"KIM/src/asymptotics/FLR2_asymptotics.f90",
"KIM/src/grid/prepare_resonances.f90",
"KIM/src/background_equilibrium/profile_input_m.f90",
"KIM/src/background_equilibrium/periodic_background.f90",
"KiLCA/flre/conductivity/calc_I_array.f90",
"KiLCA/solver/VER_5_STABLE/wave_stuff.f90",
"KiLCA/flre/conductivity/calc_I_array_drift_serg.f90",
"PreProc/fourier/src/rhs_flt.f90",
"QL-Balance/src/base/getIfunc.f90",
"QL-Balance/src/base/get_dql.f90",
"QL-Balance/src/base/rhs_balance_m.f90",
"QL-Balance/src/base/calc_current_densities.f90",
"common/equil/mag_wrapper.f90",
"common/equil/equil_profiles.f90",
"PreProc/fourier/src/fouriermodes.f90"
};
check["all indexed code paths exist", And @@ (FileExistsQ[FileNameJoin[{repositoryRoot, #}]] & /@ codePaths)];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The check name promises more than it delivers: codePaths is hand-maintained, not derived from the index.

This validates a literal list typed here, not “all indexed code paths”. I checked, and the two agree today — 22 distinct .f90 paths on both sides (24 index rows, two files serving two rows each) — so this is a drift hazard, not a present bug. But nothing enforces the correspondence: edit a path in a KIM-* row of FORMULA_INDEX.md and this check stays green, because it never reads that row.

Parsing the paths out of the index rows by regex — as codeSiteRows on line 27 already does — would make the name true and close the gap.

Comment on lines +10 to +15
chapterMaxima = <|
"1" -> 1, "2" -> 54, "3" -> 16, "4" -> 45, "5" -> 43,
"6" -> 19, "7" -> 9, "8" -> 12, "9" -> 29, "10" -> 2,
"11" -> 9, "12" -> 28, "13" -> 159, "14" -> 6, "15" -> 33,
"A" -> 29
|>;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The equation counts are asserted against a literal typed in this same file.

chapterMaxima sums to exactly 494, and line 42 then checks the index against == 494. Line 43's expectedEquations comparison derives from this same literal, as do == 24 (line 45) and == 19 (line 46). The pair proves the index is self-consistent with a hand-typed table — not that either matches the thesis. The PDF is pinned by MD5 on FORMULA_INDEX.md:7 but never opened.

#193 asks for “automated counts for thesis displayed equations”. As written the thesis count is manual, and a miscount in chapterMaxima is invisible — it would simply redefine what “complete” means and stay green.

Comment on lines +95 to +97
energyDimension = massDimension + 2 lengthDimension - 2 timeDimension;
thermalSpeedDimension = (energyDimension - massDimension)/2;
check["thermal speed has velocity units", thermalSpeedDimension === speedDimension];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This dimension check cannot see the thing C-vt locks.

C-vt (FORMULA_INDEX.md:17) says vT_sigma=sqrt(T_sigma*ev/m_sigma) “with no factor of two”. A factor of two is dimensionless, so sqrt(2T/m) passes this check identically. The registered definition on line 76 is a string and nothing evaluates it.

Same structural gap as the Fourier checks: the lock is prose, the check is arithmetic on literals, and the two never meet. Since the sqrt(2) convention split is exactly the kind of thing #193 exists to prevent, this one is worth making real.

magneticFieldDimensionCGS = {1/2, -1/2, -1};
speedDimension = lengthDimension - timeDimension;
gyroDimension = chargeDimensionCGS + magneticFieldDimensionCGS - massDimension - speedDimension;
check["CGS gyrofrequency has inverse-time units", gyroDimension === -timeDimension];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#193 asks to lock “gyrofrequency and Larmor-radius signs”. This asserts inverse-time units; the sign is untested.

C-gyro (FORMULA_INDEX.md:18) locks a signed omega_c_sigma=Zspec*e_charge*abs(B0)/(m_sigma*c) and a non-negative rho_L=abs(vT/omega_c). Both are checkable properties: that the sign of Zspec propagates into omega_c (electron negative), and that rho_L is non-negative regardless. Larmor radius doesn't appear anywhere in the script.

If[negativeFixture === "duplicate-convention",
registerConvention["phase", "exp(-i(m theta+n phi)-i omega t)", "negative fixture"]
];
check["eight executable convention definitions registered", Length[registeredConventions[]] == 8];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

“executable convention definitions” overstates what registerConvention does — see the note on checklib.wl. It asserts the strings are non-empty and that duplicates agree; a convention registered with a wrong definition passes.

Separately, this is 8 of the 19 conventions in the ledger. C-reality, C-coords, C-field, C-wavevectors, C-flow, C-bessel, C-pdf, C-collision, C-velocity-measure, C-boundary and C-parity are prose-only rows with neither registration nor check. #193 explicitly names coordinate and field orientations, velocity-space measure, Bessel, plasma-dispersion, collision-frequency normalization and boundary/gauge among the conventions to lock, so this is a spec gap rather than a naming quibble.

Comment on lines +26 to +42
registerConvention[name_String, definition_String, source_String] := Module[{},
If[StringLength[StringTrim[name]] == 0 ||
StringLength[StringTrim[definition]] == 0 ||
StringLength[StringTrim[source]] == 0,
check["classified convention " <> name, False];
Return[False]
];
If[KeyExistsQ[$conventionDefinitions, name],
Return[check[
"compatible duplicate convention " <> name,
SameQ[$conventionDefinitions[name], definition]
]]
];
AssociateTo[$conventionDefinitions, name -> definition];
AssociateTo[$conventionSources, name -> source];
True
];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

registerConvention only tests that the three strings are non-empty and that a re-registration under the same name is string-identical. It never evaluates the definition, so it cannot tell a correct convention from a wrong one — registerConvention["vT", "sqrt(42 T/m)", …] passes.

That's a perfectly good duplicate detector, which is one of the negative fixtures #193 asks for, and it demonstrably works. The only problem is the claim made of it at 01_conventions_and_inventory.wl:85 (“executable convention definitions”) — these are registry entries, not executable checks. Either rename the check, or give registerConvention a definition it can evaluate.

Comment thread Makefile

verify-mathematica-inventory:
@test -n "$(WOLFRAM_KERNEL)" || { echo "WolframKernel not found; set WOLFRAM_KERNEL"; exit 1; }
$(WOLFRAM_KERNEL) -noinit -noprompt -script verification/mathematica/01_conventions_and_inventory.wl

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AGENTS.md → Coding Style: “Line width: 100 columns maximum”. This line is 101 columns; line 53 is 126. Only one pre-existing line in this Makefile breaches the limit, so it isn't established practice here.

(01_conventions_and_inventory.wl:72 at 110 and checklib.wl:4 at 108 are over too. FORMULA_INDEX.md has ~42 long lines, but those are markdown tables that resist wrapping — I'd leave them.)

Comment thread Makefile
Comment on lines +51 to +58
verify-mathematica-inventory-negative:
@for fixture in missing-thesis missing-code duplicate-convention unclassified; do \
if KAMEL_VERIFY_NEGATIVE_FIXTURE=$$fixture $(MAKE) --no-print-directory verify-mathematica-inventory >/dev/null 2>&1; then \
echo "negative fixture unexpectedly passed: $$fixture"; exit 1; \
else \
echo "negative fixture rejected: $$fixture"; \
fi; \
done

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This gate never runs automatically. rg 'verify-mathematica' .github returns nothing — neither new target is in any workflow.

AGENTS.md → Testing: “New tests: Add via CMake in relevant subproject… register with add_test in CMakeLists.txt.” The one sanctioned exception, test/golden/, is explicitly carved out in AGENTS.md and wired into .github/workflows/golden.yml. This is neither. Combined with the tautology problem, a gate that also doesn't run is doubly cosmetic — fixing the checks matters much less if nothing ever executes them.

Shape note: golden: delegates to test/golden/run_golden.sh rather than inlining logic. A verification/run_checks.sh analogue would match that precedent and lift this 7-line shell loop out of the Makefile.

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