Classify JOREK factor-2 recovered topology - #69
Conversation
ab97096 to
e2a0584
Compare
5e9cfc8 to
b798c23
Compare
There was a problem hiding this comment.
Review verdict: Approve
Review: PR #69 — Classify JOREK recovered topology
Summary: This PR extends the test_jorek_global_plane_linearization test to classify the topological relationship between a source element and the element recovered by the global plane (the triangle's parent element). It adds a triangle_parent map (precomputed once instead of per-maximum lookup), a parent_relation classifier (same/face/vertex/remote), recovered-topology aggregate output (CSV with count, above-2%, max/rms error), and an optional "tail" CSV for high-error recovered samples. The CMakeLists wires the two new output files (topology, tail) for the factor-2 golden test.
Findings:
-
major
SRC/TESTS/test_jorek_global_plane_linearization.f90:530— Inverify_factor2_maximum, the assertion usesmaxval(abs(recovered_relation_metrics(1, :) - [...])) > tolerance)where the closing]appears after)). While this compiles (the array constructor spans lines), the placement is fragile and hard to read. More importantly, the hardcoded expected values ([4, 5987, 5987, 5987]for relation metrics,[4, 297216, 2, 47088, 20]for counts) tightly couple the test to a single golden restart. If the golden restart changes, this test breaks with no diagnostic guidance. Impact: brittle golden test. Fix: consider parameterizing or documenting that these are fixture-locked values, and add a descriptive error message identifying which quantity changed (as done for the fixture checks above). -
minor
SRC/TESTS/test_jorek_global_plane_linearization.f90:499—build_triangle_parentsiterates over all elements × cells and assignstriangle_parent(valid_triangle)but only checksvalid_triangle /= size(triangles,1)at the end. If any triangle has coincident nodes (thecycleskip), the index assignment still incrementsvalid_triangle— sotriangle_parentis filled in triangle order, which matchestrianglesorder. This is correct but subtle; a comment explaining the ordering guarantee betweentriangle_parentandtriangleswould help maintainers. -
minor
CMakeLists.txt:122-125— The factor-2 test now passes 6 output files (holes, overlaps, maxima, topology, tail) but theboundary8test at line 130 only passes 4 (holes, overlaps, maxima). The boundary test does not get topology/tail outputs. This is intentional (boundary mode skips thecompare_fieldpath that populatesrecovered_relation_*), but worth confirming this asymmetry is desired since the same executable handles both modes. -
minor
SRC/TESTS/test_jorek_global_plane_linearization.f90:622—write_topology_outputandwrite_recovered_tailopen files withnewunitbut never explicitly handleiostatfailure after open (they do checkierrand error stop, which is good). No issue, but the pattern ofopen(..., iostat=ierr)followed byif (ierr /= 0) error stopis consistently applied across all new file opens — good practice.
Verdict: Approve — The change is well-structured and correct. The test logic is sound: precomputing triangle_parent replaces the old per-call find_triangle_element (which was O(n_elements) per maximum) with an O(1) lookup. The relation classification and aggregate outputs are useful diagnostic additions. The hardcoded fixture values are consistent with a golden-test pattern already used throughout this file. No blockers found; the findings are style/maintainability suggestions.
There was a problem hiding this comment.
Review verdict: Approve
Summary
PR #69 extends the test_jorek_global_plane_linearization test with recovered-topology diagnostics: it builds a triangle_parent map (replacing per-triangle find_triangle_element searches), classifies each recovered sample's source-to-parent relation (same/face/vertex/remote), aggregates error metrics per relation class, and writes new topology and tail CSV outputs. The CMakeLists factor2 test is updated to pass the two new output paths. All changes are confined to the test harness; no production code is touched.
Findings
-
minor
test_jorek_global_plane_linearization.f90:485-506—build_triangle_parentsassumes the nested-loop iteration order (candidate → cell_j → cell_i → split) exactly matches the order in whichtriangleswas populated byextract_refined_jorek_plane. This is the same ordering assumption already present in the basefind_triangle_element, so it is not a regression; however, ifextract_refined_jorek_planeever changes its triangle emission order, both the old and new code would silently mis-assign parents. Thevalid_triangle /= size(triangles,1)guard catches count mismatches but not ordering mismatches; the hardcoded golden values inverify_factor2_maximum([4, 297216, 47088, 47088, 20],[0, 43, 12, 7]) provide the real safety net. No change required, but a comment documenting the ordering contract would help future maintainers. -
minor
test_jorek_global_plane_linearization.f90:519-535—write_recovered_tailrecomputesparent,relation, anddistanceindependently fromrecord_recovered_relation, which already computedrelation. This is a minor redundancy (extraparent_relationandneighbor_distancecalls per high-error sample) but does not affect correctness. Not blocking.
Verdict
Approve — The diff is a clean, test-only enhancement. Golden-value assertions in verify_factor2_maximum guard the new relation counts/metrics; the partition invariant sum(recovered_relation_counts) == neighbor_recovered guards all refinements. The CMake argument count (6) and the Fortran gate (>= 6 for topology, == 7 for tail) are consistent. No blockers identified.
Summary
This remains a plane-only diagnostic. Plane and volume topology, triangle selection, field interpolation, vertex ownership,
find_tetra, coordinate conventions, CGS units, and orbit behavior are unchanged.Factor-2 result
The 344,328 recovered phase samples split as follows:
The 62 above-gate phase samples reduce to 18 unique poloidal points in 13 source elements: 13 points from eight source elements use face-neighbor triangles, three points from three sources use vertex-only triangles, and two points from two sources use remote triangles. The remote tail has graph distances two and three. The limiting sample from #68 is the distance-three remote case from source element 5175 to triangle parent 5987.
The tail is highly localized around source elements 5061-5063, 5174-5175, and 5986-5988 plus their immediate partners, but it is not confined to one topological relation. A boundary-only correction, uniform factor 4, or a corner-only fan therefore does not follow from the evidence. The next diagnostic should identify which source-cell chord each of the 18 points crosses and construct the minimal conforming shared-edge closure before another plane prototype.
Outputs
jorek_global_topology_factor2.csvcontains one summary row per relation.jorek_global_tail_factor2.csvcontains the 62 above-gate rows with source coordinates, phase, containing triangle and parent, graph distance, barycentric weights, node and owner identities, and exact and interpolated(B_R,B_phi,B_Z)in gauss.Relation to the stacked work
This PR is stacked on #68 and uses its exact-maximum record.
No refinement above factor 2 enters a volume mesh in this stack.
Verification
Before, the diagnostic could not produce the topology and tail records:
After: