celeritas icon indicating copy to clipboard operation
celeritas copied to clipboard

Remove hidden state from VecGeom

Open sethrj opened this issue 1 month ago • 3 comments

Chained on #2114 , #2115

As part of #2111 I wanted to eliminate the it-could-almost-be-useful fLastExited path that lives inside the VecGeom 2 NavState but is inaccessible from VG1 and not present in the NavPath implementation. We need something like it to fix #1921 because the normal may live on the parent or the child (before or after). So to avoid keeping multiple exiting/entering states, since we already have "next" and "current", I added some wrappers that look like a navstate but operate on references to the state/boundary values. (Storage is also easier and more efficient with these since boundary bools live side by side.)

Lo and behold, several of the vecgeom tests no longer needed the exception "is solid && vecgeom 2" added in #1720. The only difference is that we discard fLastExited after every VecGeom navigation call. This indicates the new logic for excluding volumes previously entered/exited is interfering with how we do tracking.

The next step after this will be merging the solid BVH navigators.


Implementation notes

  • CELER_VGNAV is defined based on vecgeom configuration to CELER_VGNAV_PATH, CELER_VGNAV_TUPLE, or CELER_VGNAV_INDEX. The "path" selection is unfortunately deployed on all vg1 CPU and has special treatment for its state allocation.
  • Vecgeom surface navigation does not need the 'next boundary' flag since it already has 'next surface'. We use that instead, but for the moment to keep the interface consistent still allocate a 'next boundary'.
  • VgNavStateWrapper has the interface of a VG nav state, but it lacks the "last exited" state, and it simply forwards calls to the stored references to state/boundary. This lets us use it to do things like Push/Pop
  • For compatibility with other vecgeom functions that require the whole big state, there's a ScopedVgNavState that allows the full states to be saved on the stack and reconstruct the pointed-to data on destruction (discarding fLastExited in the process).
  • Use Collection instead of VecgeomNavCollection for all types except for the navigation path.

sethrj avatar Nov 18 '25 01:11 sethrj

Codecov Report

:x: Patch coverage is 89.36170% with 5 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 84.89%. Comparing base (e5b85e6) to head (9c48bef). :warning: Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
src/geocel/vg/detail/VecgeomNavCollection.hh 58.33% 5 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2116      +/-   ##
===========================================
+ Coverage    84.86%   84.89%   +0.02%     
===========================================
  Files         1253     1253              
  Lines        44135    44134       -1     
  Branches     16494    16646     +152     
===========================================
+ Hits         37456    37468      +12     
- Misses        4714     4875     +161     
+ Partials      1965     1791     -174     
Files with missing lines Coverage Δ
src/corecel/OpaqueId.hh 89.83% <ø> (ø)
src/geocel/vg/VecgeomData.hh 87.75% <100.00%> (+0.57%) :arrow_up:
src/geocel/vg/VecgeomParams.cc 65.44% <100.00%> (ø)
src/geocel/vg/VecgeomParams.hh 84.61% <ø> (ø)
src/geocel/vg/VecgeomTrackView.hh 82.41% <100.00%> (+0.91%) :arrow_up:
src/geocel/vg/detail/BVHNavigator.hh 83.47% <100.00%> (-0.70%) :arrow_down:
src/geocel/vg/detail/VecgeomNavCollection.cc 100.00% <100.00%> (+56.25%) :arrow_up:
src/geocel/vg/detail/VecgeomNavCollection.hh 57.14% <58.33%> (-19.79%) :arrow_down:

... and 131 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Nov 18 '25 02:11 codecov[bot]

Test summary

 5 702 files   9 168 suites   18m 2s :stopwatch:  2 080 tests  2 054 :white_check_mark:  26 :zzz: 0 :x: 31 837 runs  31 689 :white_check_mark: 148 :zzz: 0 :x:

Results for commit 9c48befd.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Nov 18 '25 02:11 github-actions[bot]

Regarding the NavIndexPath option for CPU, since it is not compatible with GPU, it gets automatically "upgraded" out of the *Path option when GPU is enabled.

mrguilima avatar Nov 18 '25 18:11 mrguilima

@mrguilima This is ready for review.

sethrj avatar Nov 25 '25 14:11 sethrj

Ugh, bad news: as of this commit, vecgeom 2 fails

with config:

{"_category":"system","_label":"build","config":{"build_type":"relwithdebinfo,shared","core_geo":"VecGeom","core_rng":"xorwow","debug":true,"geant4":"global-dynamic,multithreaded,shared,gdml","gpu_architectures":"90","hostname":"hudson","openmp":"disabled","real_type":"double","units":"CGS","use":["covfie","cuda","geant4","hepmc3","root","vecgeom"],"vecgeom":"bvh_single,tuple,Scalar","versions":{"CLHEP":"2.4.7.1","CUDA":"12.9.86","G4VG":"1.0.5","Geant4":"11.3.2","HepMC3":"3.03.01","ROOT":"6.36.04","Thrust":"","VecGeom":"2.0.0-dev.7","covfie":"0.15.3"}},"version":"0.7.0-dev.204+71736034a"}

before:

The following tests FAILED:
	232 - celeritas/geo/Geometry:SimpleCms* (Failed)        gpu unit
	234 - celeritas/geo/Geometry:ThreeSpheres* (Failed)     gpu unit

after (with a patch to fix missing vecgeom IWYU):

The following tests FAILED:
	 87 - geocel/vg/Vecgeom:SimpleCmsVgdmlTest.* (Subprocess aborted) gpu unit
	232 - celeritas/geo/Geometry:SimpleCms* (Failed)        gpu unit
	234 - celeritas/geo/Geometry:ThreeSpheres* (Failed)     gpu unit
	317 - celeritas/track/TrackSort (Failed)                gpu unit
	323 - celeritas/user/SlotDiagnostic (Failed)            gpu unit
	354 - accel/TrackingManagerIntegration:LarSphere.run:gpu:mt (Failed) gpu unit
	355 - accel/TrackingManagerIntegration:LarSphere.run:gpu:task (Failed) gpu unit

Looks like this is because of my hacky (incorrect+illegal) use of NavStateTuple as POD.

sethrj avatar Dec 01 '25 18:12 sethrj