Skip to content

Cover the lasym cos part of the vacuum grpmn and bvec - #738

Merged
jurasic-pf merged 3 commits into
proximafusion:mainfrom
CharlesCNorton:lasym-vacuum-tests
Sep 7, 2026
Merged

Cover the lasym cos part of the vacuum grpmn and bvec#738
jurasic-pf merged 3 commits into
proximafusion:mainfrom
CharlesCNorton:lasym-vacuum-tests

Conversation

@CharlesCNorton

@CharlesCNorton CharlesCNorton commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Change - covers the cos halves of grpmn and bvec in the NESTOR vacuum tests, on a new asymmetric free-boundary case, and drops the four TODO(jons) markers that stood in for that coverage.

Cause - grpmn and bvec carry a second symmetry index for lasym, but educational_VMEC dumped only the first slice; analyt.f90 notes it at the call as ! missing dim: (ndim: 1 or 2). With no reference for the cos half, the asymmetric vacuum path was untested. jonathanschilling/educational_VMEC#30 writes it out.

Evidence - the reference is a free-boundary lasym = T cth-like case at ns = 15, mpol = 5, ntor = 4, nfp = 5, generated with the mgrid VMEC++ already uses for cth_like_free_bdy_asym, md5-identical, and with every namelist parameter matching cth_like_free_bdy_asym.json. Both halves agree at 1e-9, the same tolerance the symmetric case uses.

Tests - FourPTest, FourIAccumulateGrpmnTest and AnalytTest gain cth_like_free_bdy_asym at iteration 53 and compare grpmn_cos and bvec_cos where lasym is set. Both C++ suites pass, 44/44.

Scope - test-only. The symmetric cases are unchanged and the new comparisons are skipped when lasym is false. Needs educational_VMEC#30 for anyone regenerating the reference; the JSONs are checked in, so the tests do not depend on it.

@jurasic-pf jurasic-pf 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.

These comments are quire hard to read. The code should still be understandable and maintainable by humans, and not just ai.

@CharlesCNorton

CharlesCNorton commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@jurasic-pf Rewriting them. Separately, #821 proposes fixing a layout for PR descriptions in AGENTS.md, for the same reason.

@CharlesCNorton CharlesCNorton changed the title Record what the missing lasym cos-part reference leaves uncovered Cover the lasym cos part of the vacuum grpmn and bvec Sep 7, 2026
@CharlesCNorton

Copy link
Copy Markdown
Contributor Author

@jurasic-pf Rather than reword the markers I removed the reason for them: educational_VMEC now writes the cos half (jonathanschilling/educational_VMEC#30), and the tests compare it on a new asymmetric free-boundary case at 1e-9. Retitled, since the PR no longer records a gap.

@CharlesCNorton
CharlesCNorton force-pushed the lasym-vacuum-tests branch 3 times, most recently from 5d7ac1b to 64579a6 Compare September 7, 2026 14:13

@jurasic-pf jurasic-pf 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.

Perfect! With test coverage and easy to read code

@jurasic-pf
jurasic-pf enabled auto-merge (squash) September 7, 2026 23:02
@jurasic-pf
jurasic-pf merged commit 0618c40 into proximafusion:main Sep 7, 2026
35 checks passed
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.

2 participants