Skip to content

Classify JOREK factor-2 recovered topology - #69

Open
krystophny wants to merge 1 commit into
test/jorek-global-error-maximafrom
test/jorek-recovered-topology
Open

Classify JOREK factor-2 recovered topology#69
krystophny wants to merge 1 commit into
test/jorek-global-error-maximafrom
test/jorek-recovered-topology

Conversation

@krystophny

Copy link
Copy Markdown
Member

Summary

  • map every complete-plane triangle back to its JOREK parent element without changing triangle order or orientation
  • classify every factor-2 neighbor-recovered phase sample as same-element, face-neighbor, vertex-only, or remote
  • export relation counts, maximum and RMS field errors, the population above the 2% gate, and one detailed row for each above-gate sample
  • freeze the factor-2 relation counts and maxima as regression fixtures

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:

Triangle-parent relation Count Above 2% Maximum RMS
Same source element 4 0 0.517504% 0.515103%
Face neighbor 297,216 43 3.364268% 0.481686%
Vertex-only neighbor 47,088 12 2.886275% 0.485997%
Remote 20 7 4.165709% 2.455348%

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.csv contains one summary row per relation. jorek_global_tail_factor2.csv contains 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:

$ test_jorek_global_plane_linearization.x restart.h5 2 holes.csv overlaps.csv maxima.csv topology.csv tail.csv
ERROR STOP restart path, refinement, and optional output paths are required
exit code 1

After:

$ ctest --test-dir /tmp/gorilla-jorek-adaptive -R 'test_jorek_global_plane_linearization_' --output-on-failure
100% tests passed, 0 tests failed out of 5
Total Test time (real) = 16.66 sec

$ ctest --test-dir /tmp/gorilla-jorek-adaptive --output-on-failure
100% tests passed, 0 tests failed out of 17
Total Test time (real) = 49.59 sec

$ wc -l /tmp/gorilla-jorek-adaptive/jorek_global_tail_factor2.csv
63 /tmp/gorilla-jorek-adaptive/jorek_global_tail_factor2.csv

$ fo
Static: OK (78 modules, 78 changed, 78 affected)
Build: OK
Tests: skipped, no affected tests
Lint: OK
All stages passed (.1s)

@krystophny
krystophny force-pushed the test/jorek-recovered-topology branch from 5e9cfc8 to b798c23 Compare July 17, 2026 11:26

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

  1. major SRC/TESTS/test_jorek_global_plane_linearization.f90:530 — In verify_factor2_maximum, the assertion uses maxval(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).

  2. minor SRC/TESTS/test_jorek_global_plane_linearization.f90:499build_triangle_parents iterates over all elements × cells and assigns triangle_parent(valid_triangle) but only checks valid_triangle /= size(triangles,1) at the end. If any triangle has coincident nodes (the cycle skip), the index assignment still increments valid_triangle — so triangle_parent is filled in triangle order, which matches triangles order. This is correct but subtle; a comment explaining the ordering guarantee between triangle_parent and triangles would help maintainers.

  3. minor CMakeLists.txt:122-125 — The factor-2 test now passes 6 output files (holes, overlaps, maxima, topology, tail) but the boundary8 test at line 130 only passes 4 (holes, overlaps, maxima). The boundary test does not get topology/tail outputs. This is intentional (boundary mode skips the compare_field path that populates recovered_relation_*), but worth confirming this asymmetry is desired since the same executable handles both modes.

  4. minor SRC/TESTS/test_jorek_global_plane_linearization.f90:622write_topology_output and write_recovered_tail open files with newunit but never explicitly handle iostat failure after open (they do check ierr and error stop, which is good). No issue, but the pattern of open(..., iostat=ierr) followed by if (ierr /= 0) error stop is 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.

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

  1. minor test_jorek_global_plane_linearization.f90:485-506build_triangle_parents assumes the nested-loop iteration order (candidate → cell_j → cell_i → split) exactly matches the order in which triangles was populated by extract_refined_jorek_plane. This is the same ordering assumption already present in the base find_triangle_element, so it is not a regression; however, if extract_refined_jorek_plane ever changes its triangle emission order, both the old and new code would silently mis-assign parents. The valid_triangle /= size(triangles,1) guard catches count mismatches but not ordering mismatches; the hardcoded golden values in verify_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.

  2. minor test_jorek_global_plane_linearization.f90:519-535write_recovered_tail recomputes parent, relation, and distance independently from record_recovered_relation, which already computed relation. This is a minor redundancy (extra parent_relation and neighbor_distance calls 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.

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