Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support surface-based VecGeom 2.x navigator #1422

Draft
wants to merge 36 commits into
base: develop
Choose a base branch
from

Conversation

mrguilima
Copy link
Contributor

@mrguilima mrguilima commented Sep 23, 2024

Adapting Celeritas to use the latest surface-based model from VecGeom 2.x (see #1412).

The main change is to use VecGeom's vgbrep::protonav::BVHSurfNavigator. Since there are interface differences between this one and the volume-based BVHNavigator, the code-guards VECGEOM_USE_SURF, in analogy to AdePT.
Alternatively, VECGEOM_VERSION >= 0x020000 could be used instead, as the VecGeom 1.2.x series should always be used for volume-based navigation anyway.

There are also distinct interfaces for ABBoxManager (template vs. non-template).

Note: in draft mode, as some tests currently fail. Under investigation.

@mrguilima mrguilima requested a review from sethrj September 23, 2024 17:24
@mrguilima mrguilima self-assigned this Sep 23, 2024
@mrguilima mrguilima added the enhancement New feature or request label Sep 23, 2024
@mrguilima mrguilima closed this Sep 23, 2024
@mrguilima mrguilima reopened this Sep 23, 2024
@sethrj sethrj added the external Dependencies and framework-oriented features label Sep 23, 2024
@sethrj sethrj marked this pull request as draft September 23, 2024 17:50
@sethrj sethrj changed the title Adapt Celeritas to use surface-based VecGeom 2.x navigator *[enhancement, geometry]* Support surface-based VecGeom 2.x navigator Sep 23, 2024
@@ -11,6 +11,7 @@
#include <vector>
#include <VecGeom/base/Config.h>
#include <VecGeom/base/Cuda.h>
#include <VecGeom/base/Version.h>
Copy link
Member

Choose a reason for hiding this comment

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

I've verified that this header (and the VECGEOM_VERSION macro therein) are present in v1.2.4, which is the minimum vecgeom required by Celeritas.

globalpoint, globaldir, in_state, out_state, exit_surf, step_limit);
long hitsurf_id = 0;
auto step = vgbrep::protonav::BVHSurfNavigator<Precision>::ComputeStepAndNextSurface(
globalpoint, globaldir, in_state, out_state, hitsurf_id, step_limit);
Copy link
Member

Choose a reason for hiding this comment

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

Something here is misbehaving compared to the volume version:

54: /Users/seth/Code/celeritas-temp/test/geocel/vg/Vecgeom.test.cc:164: Failure
54: Value of: next.boundary
54:   Actual: true
54: Expected: false

for the calls

    auto next = geo.find_next_step(from_cm(100));
    EXPECT_SOFT_EQ(100, to_cm(next.distance));
    EXPECT_FALSE(next.boundary);

So it's claiming to have a boundary for the next surface, even though it doesn't hit it within the step limit.

Copy link

github-actions bot commented Sep 24, 2024

Test summary

  268 files    404 suites   32s ⏱️
1 290 tests 1 279 ✅ 8 💤 3 ❌
1 312 runs  1 304 ✅ 5 💤 3 ❌

For more details on these failures, see this check.

Results for commit b7a5aaa.

♻️ This comment has been updated with latest results.

@sethrj
Copy link
Member

sethrj commented Sep 25, 2024

Without GPU on my laptop (clang 15 debug), on VecGeom ca34e35ea5fc822fcffd3c7c5d117e6bc671ce32 (reldeb), the following tests fail:

	 54 - geocel/vg/Vecgeom:SimpleCmsTest.* (Failed)
	 55 - geocel/vg/Vecgeom:FourLevelsGeantTest.* (Failed)
	 56 - geocel/vg/Vecgeom:SolidsGeantTest.* (SEGFAULT)
	 57 - geocel/vg/Vecgeom:ZnenvGeantTest.* (Failed)
	166 - celeritas/field/FieldPropagator (SEGFAULT)
	167 - celeritas/field/LinearPropagator (Failed)
	169 - celeritas/geo/Geometry (SEGFAULT)
	170 - celeritas/geo/GeoMaterial (Failed)
	171 - celeritas/global/AlongStep:-Em3*:SimpleCms*:LeadBox* (SEGFAULT)
	172 - celeritas/global/AlongStep:Em3AlongStepTest.nofluct_nomsc (Failed)
	173 - celeritas/global/AlongStep:Em3AlongStepTest.msc_nofluct (Failed)
	175 - celeritas/global/AlongStep:Em3AlongStepTest.fluct_nomsc (Failed)
	176 - celeritas/global/AlongStep:SimpleCmsAlongStepTest.msc_field (Subprocess killed)
	178 - celeritas/global/AlongStep:SimpleCmsRZFieldAlongStepTest.msc_rzfield (Failed)
	180 - celeritas/global/KernelContextException (Failed)
	181 - celeritas/global/Stepper (Failed)
	182 - celeritas/global/StepperGeant:-TestEm*:LeadBox* (Failed)
	183 - celeritas/global/StepperGeant:TestEm3Compton.* (Failed)
	184 - celeritas/global/StepperGeant:TestEm3NoMsc.* (Failed)
	185 - celeritas/global/StepperGeant:TestEm3Msc.* (Failed)
	186 - celeritas/global/StepperGeant:TestEm3MscNofluct.* (Failed)
	187 - celeritas/global/StepperGeant:TestEm15FieldMsc.* (Failed)
	203 - celeritas/optical/OpticalCollector (Failed)
	238 - celeritas/track/TrackSort (Failed)
	241 - celeritas/user/Diagnostic:-TestEm3* (Failed)
	242 - celeritas/user/Diagnostic:TestEm3* (Failed)
	243 - celeritas/user/StepCollector (SEGFAULT)

The simplest failure is just that it seems to report aa boundary when not actually hitting a boundary:

[ RUN      ] SimpleCmsTest.track
/Users/seth/Code/celeritas-temp/test/geocel/vg/Vecgeom.test.cc:164: Failure
Value of: next.boundary
  Actual: true
Expected: false

/Users/seth/Code/celeritas-temp/test/geocel/vg/Vecgeom.test.cc:176: Failure
Expected equality of these values:
  "si_tracker"
  this->volume_name(geo)
    Which is: "world"

[  FAILED  ] SimpleCmsTest.track (0 ms)

Full failure log:
LastTest.log

test/geocel/vg/Vecgeom.test.cc Outdated Show resolved Hide resolved
0.52499999999986, 13.023518051922, 6.95, 6.95, 13.023518051922,
0.52499999999986, 2.15, 100, 5, 8, 100, 100, 100};
10.3027302206745, 13.023518051922, 6.95, 6.95, 13.023518051922,
10.3027302206745, 2.15, 100, 5, 8, 100, 100, 100};
Copy link
Member

Choose a reason for hiding this comment

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

It's great that you've recorded the updated safety values: these are now consistent with the ones returned by Geant4. I suppose we'll have to "edit" these when using the volume-based vecgeom.

@@ -1034,7 +1120,8 @@ TEST_F(CmseTest, trace)
static real_type const expected_distances[] = {12.495, 287.505, 530,
920};
EXPECT_VEC_SOFT_EQ(expected_distances, result.distances);
static real_type const expected_hw_safety[] = {1, 1, 242, 460};
static real_type const expected_hw_safety[] = {6.2475, 47.9499999999998,
242, 460};
Copy link
Member

Choose a reason for hiding this comment

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

Guess we'll have to change the label on this one 😄 since it now agrees between the two

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external Dependencies and framework-oriented features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants