Skip to content

macOS CPU build + toroid phantom-collision fix (#467) - #469

Open
treegardel wants to merge 4 commits into
masterfrom
macos-cpu-build
Open

macOS CPU build + toroid phantom-collision fix (#467)#469
treegardel wants to merge 4 commits into
masterfrom
macos-cpu-build

Conversation

@treegardel

@treegardel treegardel commented Jun 19, 2026

Copy link
Copy Markdown
Member

Type of Change

  • Bug fix (non breaking change, fixing an issue)
  • New feature
  • Breaking change (fix or feature that causes existing functionality to not work as expected)
  • Documentation / Wiki update

Description

This PR bundles two related sets of fixes that came up while getting the macOS CPU build green.

1. macOS CPU build pipeline

  • Set macOS deployment target to 13.3 and pass it consistently to CMake.
  • Disable rayx-ui and CUDA only for macOS CI.
  • Update CMake Apple logic so CMAKE_OSX_DEPLOYMENT_TARGET is only set as fallback (does not override workflow value).
  • Add AppleClang to platform detection so RAYX_API exports symbols correctly on macOS.
  • Implement macOS getExecutablePath() / getFullPath() in Locate.cpp (was static_assert(false)).
  • Drop the spurious baseDir segment from the macOS resource lookup so Data/PALIK, Data/NFF, Data/CROMER, Data/MOLEC are found next to the executable (matches Linux behavior). Without this, all material tables came back empty and getRefractiveIndex returned (-1, -1).
  • Fix formatAsVec redefinition on Linux (unsigned long == RandCounter).

2. Toroid phantom collisions — closes #467

The Newton solver in getToroidCollision converges to within NEW_TOLERANCE = 1e-4 mm. After a hit, the ray is nudged forward by COLLISION_EPSILON = 1e-6 mm — much less than the Newton residual. The forward check tested only the sign:

if (dot(rayToHitpoint, rayDirection) <= 0.0) return std::nullopt;

A solution within the residual on the wrong side of the new origin still passed, so each toroid bounce was counted ~4× in non-sequential tracing.

Fix: reject hits inside Newton's own uncertainty by checking parametric forward distance, local to the toroid solver:

const double t = dot(rayToHitpoint, rayDirection) / dot(rayDirection, rayDirection);
if (t <= NEW_TOLERANCE) return std::nullopt;

COLLISION_EPSILON stays at 1e-6 mm, all other surfaces keep their precision.

Tests added:

  • Integration test toroid_iteration (grazing toroid vs. RAY-UI reference, regenerated at proper 1e-4 tolerance — old reference was at 1e-1 and hid the agreement).
  • Unit tests on getToroidCollision: forward & backward grazing hits at z ≈ ±600 mm walking a tolerance series from 1e-2 down to 1e-6 mm (1 nm), plus a phantom-rejection regression test. Newton's quadratic convergence delivers sub-nm precision in practice.

Bonus: getCubicCollision had 29 pow(float(expr), N) calls truncating doubles to single precision on every Newton iteration — capping cubic-surface accuracy at ~1e-7 relative regardless of tolerance. Removed; all tests still pass.

This is not a breaking change. Linux/Windows workflows are not functionally changed by this PR.

Fixes #467


✅ Pre-Merge Checklist

  • Code follows the project's coding standards
  • Unit tests for new functionality are added and pass
  • All existing tests pass
  • Resolved TODO Comments (prefer new issues instead)
  • Documentation, if applicable, including:
    • Doxygen comments for any new rayx-core API functions (N/A)
    • Helpful inline comments where needed for clarity
    • Wiki pages, e.g. updated build instructions, new Element etc. (N/A)
  • Commits:
    • Use clear and readable commit messages
    • Squash and rebase onto master if individual commits don't add value
    • Ensure linear commit history (required by master)

ℹ️ Supersedes #465 — der ursprüngliche PR wurde versehentlich gemergt und der Merge anschließend per Reset aus master entfernt. Da ein gemergter PR auf GitHub nicht wieder geöffnet werden kann, ersetzt dieser PR ihn inhaltlich unverändert.

CI:
- Add testMacOS.yaml workflow for macOS CPU build (no rayx-ui, no CUDA).
- Set MACOSX_DEPLOYMENT_TARGET=13.3 and pass it consistently to CMake.
- Install required toolchain dependencies (compiler, OpenMP, HDF5, zlib, make).

Build:
- Add AppleClang to platform detection so RAYX_API exports symbols correctly.
- Implement macOS getExecutablePath() (was static_assert(false)) using _NSGetExecutablePath.
- Drop spurious baseDir from the macOS resource lookup so Data/PALIK, NFF,
  CROMER, MOLEC are found next to the executable (matches Linux).
- Set CMAKE_OSX_DEPLOYMENT_TARGET only as fallback so it does not override
  the workflow value.

Source / tests:
- Fix formatAsVec redefinition on Linux (RandCounter == unsigned long there).
- Add CPU-build fallback for the test runner so it works without GPU build.
- Reduce noisy tolerances in testSchwingerDipole, testBessel1, PlaneGratingDevAzMisVLS.
@treegardel
treegardel requested a review from JonasTrenkler June 19, 2026 13:51
treegardel and others added 3 commits June 23, 2026 10:05
The Newton solver in getToroidCollision converges to within
NEW_TOLERANCE = 1e-4 mm. After a hit, the ray is nudged forward by
COLLISION_EPSILON = 1e-6 mm — much less than the Newton residual. The
forward check tested only the sign, so a solution within the residual
on the wrong side of the new origin still passed and each toroid
bounce was counted ~4x in non-sequential tracing.

Reject hits inside Newton's own uncertainty by checking parametric
forward distance, local to the toroid solver:

    const double t = dot(rayToHitpoint, rayDirection)
                     / dot(rayDirection, rayDirection);
    if (t <= NEW_TOLERANCE) return std::nullopt;

COLLISION_EPSILON stays at 1e-6 mm, all other surfaces keep their
precision.

Tests:
- toroid_iteration integration test (grazing toroid vs. RAY-UI
  reference, regenerated at proper 1e-4 tolerance — old reference
  was at 1e-1 and hid the agreement). Now matches to ~1e-10 mm.
- Three unit tests on getToroidCollision: forward and backward grazing
  hits at z ~ +/-600 mm, plus a phantom-rejection regression test.
  Newton's quadratic convergence delivers sub-nm precision in practice;
  the tests assert only the guaranteed 1e-4 bound and log the finer value.
- Re-enable ReflectionZonePlateDefault200Toroid with Sequential::Yes.

Also tidied getCubicCollision: stripped redundant parentheses in ~29
pow((expr), N) calls (pow((xx), 2.0) -> pow(xx, 2.0)). Purely cosmetic —
both forms call std::pow on doubles; no numerical change to the solver.

Also export getToroidCollision via RAYX_API so the test binary can call
it directly.
- Drop the INSTALL_DATA_DIR override in Intern/rayx-core/CMakeLists.txt that
  clobbered the platform-specific value computed at top level (and broke
  RAYX_CUSTOM_DATA_DIR) on all platforms.
- Reduce the APPLE block to OpenMP/HDF5 hint variables only; let the gated
  find_package calls in Extern/CMakeLists.txt do discovery (honoring
  RAYX_ENABLE_*/RAYX_REQUIRE_*) and link via OpenMP::OpenMP_CXX instead of
  global flags / double libomp linking.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The forward/backward grazing-hit unit tests asserted err <= 1e-6 mm, but the
Newton solver in getToroidCollision only guarantees convergence to
NEW_TOLERANCE = 1e-4. The 1e-6 bound relied on empirically observed quadratic
convergence (~sub-nm on macOS arm64) and could flake on other platforms whose
FP path stops a few iterations earlier while still being correct. Assert only
the guaranteed 1e-4 bound; the finer precision is already logged via RAYX_LOG.

Correction to 6dbb136 (#467): its commit message claims getCubicCollision had
"pow(float(expr), N) calls truncating doubles to single precision" that were
removed. That is inaccurate -- the diff only strips redundant parentheses
(pow((xx), 2.0) -> pow(xx, 2.0)); both forms call std::pow on doubles. No
precision change happened to the cubic solver.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

getToroidCollision: Newton iteration produces phantom collisions in non-sequential tracing

1 participant