Skip to content

Fix wall_hit_cart/xend_cart inconsistency when from_cart fails#321

Merged
krystophny merged 4 commits into
mainfrom
fix/cgal-wall-footprints
Jun 6, 2026
Merged

Fix wall_hit_cart/xend_cart inconsistency when from_cart fails#321
krystophny merged 4 commits into
mainfrom
fix/cgal-wall-footprints

Conversation

@krystophny

@krystophny krystophny commented Jan 15, 2026

Copy link
Copy Markdown
Member

Risk tier

  • T0: docs, comments, small build metadata
  • T1: pure refactor, no behavior change intended
  • T2: local numerical logic
  • T3: physics, output behavior, coordinate convention
  • T4: parallelism, GPU, dependency, CI, or security-sensitive build logic

Correctness contract

Intended behavior change

For STL wall intersections, xend_cart is written from the authoritative CGAL wall_hit_cart point. If from_cart() fails in an ill-conditioned chartmap region, zend falls back to linearly interpolated reference coordinates for angle-space diagnostics.

Untraced particles now write zero xend_cart instead of reusing xstart_cart.

Behavior that must not change

Non-STL end positions still come from zend through the reference-coordinate Cartesian transform. zstart, zend, times_lost, class output, and wall-hit flags keep their existing shapes and meanings.

Coordinate / unit conventions

xend_cart and wall_hit_cart are Cartesian coordinates in the NetCDF xyz frame and units. zend remains phase-space output in reference coordinates.

Numerical invariants

For STL hits, xend_cart == wall_hit_cart at the recorded hit. For untraced particles, zend(1:3) == 0 implies xend_cart == 0.

Tests added

  • unit: none
  • integration: updated test_netcdf_results.py
  • system: focused NetCDF output run through make test TEST=test_netcdf_results VERBOSE=1
  • golden record: unchanged

Golden-record impact

  • unchanged
  • changed

Failure modes considered

  • from_cart() may fail near ill-conditioned chartmap regions.
  • Recomputing xend_cart from bad zend can move the displayed wall hit away from the true STL intersection.
  • The short Python NetCDF fixture may not produce STL hits; the assertion is present and skips when no hit is generated.

Manual validation

The NetCDF focused test passed after updating the untraced-particle expectation and adding the wall-hit semantic assertion.

Verification

$HOME/code/prompts/scripts/check-writing-slop.py test/python/test_netcdf_results.py
PASS: no writing-slop candidates at threshold medium

make test TEST=test_netcdf_results VERBOSE=1
...
../../../test/python/test_netcdf_results.py::TestNetCDFResultsOutput::test_untraced_particles_have_zero_xend PASSED [ 75%]
../../../test/python/test_netcdf_results.py::TestNetCDFResultsOutput::test_wall_hits_use_authoritative_cartesian_hit SKIPPED [ 87%]
../../../test/python/test_netcdf_results.py::TestNetCDFResultsOutput::test_netcdf_has_proper_attributes PASSED [100%]
=================== 7 passed, 1 skipped in 77.71s (0:01:17) ====================
1/1 Test #88: test_netcdf_results ..............   Passed   78.21 sec
100% tests passed, 0 tests failed out of 1

@krystophny

Copy link
Copy Markdown
Member Author

Visual Evidence

CGAL Wall Hit Position Consistency

The histogram compares position differences between wall_hit_cart (true CGAL intersection) and xend_cart (computed from reference coordinates) before and after this fix.

Key improvements:

  • Excellent agreement (<1 cm): 65.2% -> 78.6%
  • Severe discrepancy (>500 cm): 22.3% -> 0%

Test configuration: SQUID stellarator, 1ms simulation, 1024 particles starting at s=0.6, using CGAL-embedded STL chartmap.

@qodo-code-review

qodo-code-review Bot commented Jan 15, 2026

Copy link
Copy Markdown
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Supply chain integrity

Description: The build fetches and builds a third-party dependency (CGAL) directly from a remote URL
via FetchContent_Declare(URL ...) without any pinned integrity verification (e.g.,
URL_HASH/checksum or signature verification), which creates a software supply-chain risk
if the downloaded archive is tampered with or replaced upstream.
CMakeLists.txt [209-223]

Referred Code
include(FetchContent)
FetchContent_Declare(
    cgal
    URL https://github.com/CGAL/cgal/releases/download/v6.0.1/CGAL-6.0.1.tar.xz
    DOWNLOAD_EXTRACT_TIMESTAMP TRUE
)
FetchContent_GetProperties(cgal)
if(NOT cgal_POPULATED)
    FetchContent_Populate(cgal)
endif()
# CGAL 6.x is header-only, just add include path
set(CGAL_INCLUDE_DIR "${cgal_SOURCE_DIR}/include")
message(STATUS "CGAL enabled via FetchContent: 6.0.1 (header-only)")
message(STATUS "CGAL include dir: ${CGAL_INCLUDE_DIR}")
add_compile_definitions(SIMPLE_ENABLE_CGAL)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Fallback edge case: The new interpolation fallback silently does nothing when segment_length is zero, leaving
z(1:3) potentially unupdated after from_cart() failure and risking the same inconsistency
the fix aims to address.

Referred Code
segment_length = sqrt(sum((x_cur_m - x_prev_m)**2))
if (segment_length > 0.0_dp) then
    hit_distance = sqrt(sum((x_hit_m - x_prev_m)**2))
    t_frac = hit_distance / segment_length
    u_ref_hit = u_ref_prev + t_frac * (u_ref_cur - u_ref_prev)
    call ref_to_integ(u_ref_hit, z(1:3))
end if

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@krystophny

Copy link
Copy Markdown
Member Author

Update: The visual evidence histogram is available locally at /tmp/cgal_fix_comparison.png showing:

Position Discrepancy    Before Fix    After Fix
-------------------------------------------
< 1 cm (excellent)       65.2%         78.6%
> 100 cm (large)         32.1%          3.6%
> 500 cm (severe)        22.3%            0%

The fix significantly improves consistency between the CGAL-reported wall intersection (wall_hit_cart) and the position computed from reference coordinates (xend_cart).

@qodo-code-review

qodo-code-review Bot commented Jan 15, 2026

Copy link
Copy Markdown
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle zero-length segment case

Add an else branch to handle the zero-length segment case by setting u_ref_hit
to u_ref_prev, resetting ierr_from_cart, and calling ref_to_integ to prevent an
uninitialized state.

src/simple_main.f90 [634-639]

 if (segment_length > 0.0_dp) then
     hit_distance = sqrt(sum((x_hit_m - x_prev_m)**2))
     t_frac = hit_distance / segment_length
     u_ref_hit = u_ref_prev + t_frac * (u_ref_cur - u_ref_prev)
+    ierr_from_cart = 0
+    call ref_to_integ(u_ref_hit, z(1:3))
+else
+    u_ref_hit = u_ref_prev
+    ierr_from_cart = 0
     call ref_to_integ(u_ref_hit, z(1:3))
 end if
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug in the new fallback logic where a zero-length segment would leave the particle state un-updated and the error flag set, leading to incorrect behavior. The proposed fix is comprehensive and correct.

Medium
General
Clamp interpolation fraction

Clamp the interpolation fraction t_frac to the [0, 1] range to guard against
numerical errors and prevent extrapolation.

src/simple_main.f90 [636]

-t_frac = hit_distance / segment_length
+t_frac = min(1.0_dp, max(0.0_dp, hit_distance / segment_length))
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This suggestion correctly identifies a potential numerical instability where hit_distance could be slightly larger than segment_length, leading to extrapolation. Clamping t_frac makes the interpolation more robust against floating-point inaccuracies.

Low
  • Update

@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from c85adf0 to 26182df Compare March 27, 2026 17:48
@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from 26182df to 00db71a Compare May 15, 2026 19:59
@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from 00db71a to a7e8614 Compare May 15, 2026 20:00
@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from a7e8614 to b0f92bc Compare June 5, 2026 20:41
@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from b0f92bc to c2a6649 Compare June 5, 2026 23:14
@krystophny krystophny added tier/T3 physics or output behavior size/S review size up to 100 changed lines labels Jun 5, 2026
@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from c2a6649 to 7b58a67 Compare June 5, 2026 23:35
krystophny and others added 4 commits June 6, 2026 16:25
When CGAL detects a wall intersection, wall_hit_cart correctly stores
the intersection point in Cartesian coordinates. The code then calls
from_cart() to convert this back to reference coordinates for zend.

However, when from_cart() Newton iteration fails to converge (ierr != 0),
the reference coordinates z(1:3) were not updated, causing zend and
xend_cart to record the particle position PAST the wall rather than AT
the wall intersection.

This fix adds a linear interpolation fallback: when from_cart() fails,
interpolate reference coordinates along the orbit segment using the
fractional distance to the Cartesian hit point.

Also update CGAL from 5.6.1 to 6.0.1 to fix GCC compilation error with
Halfedge_around_source_iterator::base().

Test results show significant improvement:
- Before: 32.1% of wall hits had >100cm discrepancy
- After:  3.6% of wall hits have >100cm discrepancy
For STL wall hits, xend_cart should equal wall_hit_cart (the true CGAL
intersection point). Previously, xend_cart was computed from zend via
forward chartmap transformation, which could produce incorrect positions
when the earlier from_cart inverse transformation failed.

This fix ensures xend_cart is always correct for STL hits by using
wall_hit_cart directly, avoiding the round-trip coordinate conversion.

Also set xend_cart to zero (not xstart_cart) for untraced particles.

Test results:
- PRE-FIX: 2/50 exact matches, max error 1595 cm
- POST-FIX: 50/50 exact matches, zero error
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@krystophny
krystophny force-pushed the fix/cgal-wall-footprints branch from 550f1f3 to 4724600 Compare June 6, 2026 14:25
@krystophny
krystophny merged commit f04397a into main Jun 6, 2026
9 of 10 checks passed
@krystophny
krystophny deleted the fix/cgal-wall-footprints branch June 6, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 2/5 size/S review size up to 100 changed lines tier/T3 physics or output behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant