Integrate Toroidal surfaces into ORANGE contruction

[osanstrong]

This PR integrates the elliptical toroid surface from #2158 into ORANGE construction features to allow loading toroids (specifically toruses) from input files such as Geant4 gdml.

It, however, does not forward the surface type or enable it at runtime, as its performance impact has not yet been analyzed.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.31%. Comparing base (c5d2318) to head (b50220b).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/orange/orangeinp/ObjectIO.json.cc	0.00%	5 Missing ⚠️
src/orange/orangeinp/IntersectRegion.cc	83.33%	3 Missing ⚠️
src/orange/surf/detail/SurfaceTransformer.cc	0.00%	3 Missing ⚠️
src/orange/surf/SurfaceTypeTraits.hh	0.00%	2 Missing ⚠️
src/orange/detail/SurfacesRecordBuilder.cc	0.00%	1 Missing ⚠️
src/orange/orangeinp/IntersectRegion.hh	66.66%	0 Missing and 1 partial ⚠️
src/orange/surf/FaceNamer.cc	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2427      +/-   ##
===========================================
- Coverage    87.33%   87.31%   -0.02%     
===========================================
  Files         1401     1401              
  Lines        44443    44507      +64     
  Branches     13495    13517      +22     
===========================================
+ Hits         38813    38862      +49     
- Misses        4408     4417       +9     
- Partials      1222     1228       +6     

Files with missing lines	Coverage Δ
src/orange/OrangeTypes.cc	81.91% <ø> (ø)
src/orange/OrangeTypes.hh	81.25% <ø> (ø)
src/orange/g4org/SolidConverter.cc	94.82% <ø> (ø)
src/orange/orangeinp/IntersectSurfaceBuilder.cc	92.00% <ø> (ø)
src/orange/orangeinp/Solid.cc	94.04% <ø> (ø)
src/orange/orangeinp/Solid.hh	100.00% <ø> (ø)
...rc/orange/orangeinp/detail/LocalSurfaceInserter.cc	95.71% <ø> (ø)
src/orange/orangeinp/detail/SurfaceHashPoint.hh	100.00% <100.00%> (ø)
src/orange/surf/FaceNamer.hh	100.00% <ø> (ø)
src/orange/surf/SoftSurfaceEqual.cc	100.00% <100.00%> (ø)
... and 14 more

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Test summary

 5 353 files   8 741 suites   18m 26s ⏱️
 2 331 tests  2 288 ✅  43 💤 0 ❌
31 136 runs  30 980 ✅ 156 💤 0 ❌

Results for commit b50220b.

♻️ This comment has been updated with latest results.

[sethrj]

This is looking great! I know it's not "ready" yet but I have a couple of comments: mainly let's keep this PR from altering the runtime static-polymorphic surfaces.

[sethrj]

For now let's treat this like SurfaceType::inv and prevent runtime due to the unknown impact on GPU performance.

[osanstrong]

Right, sounds like a plan! Is forwarding/not the surface type here what allows/prevents it from compiled to the GPU kernel and impacting performance?

[sethrj]

Exactly. That prevents the GPU code from being compiled, and invalid surfaces are caught during construction:

celeritas/src/orange/detail/SurfacesRecordBuilder.cc

Lines 42 to 48 in c5d2318

	if constexpr (std::remove_reference_t<decltype(s)>::surface_type()
	== SurfaceType::inv)
	{
	// See discussion on
	// https://github.com/celeritas-project/celeritas/pull/1342
	CELER_NOT_IMPLEMENTED("runtime involute support");
	}

which you could turn into:

constexpr auto st = std::remove_reference_t<decltype(s)>::surface_type();
if constexpr (st == SurfaceType::inv || st == ...)

[elliottbiondo]

@osanstrong if you make a branch with runtime support I can test it very easily

[osanstrong]

@osanstrong if you make a branch with runtime support I can test it very easily

Try git@github.com:osanstrong/celeritas.git, branch torruntime (or on web here)

[elliottbiondo]

Here are the timing results compared to Owen's torruntime branch. Looks like a 1.6% slowdown on DUNE and 8.9% slowdown for ATLAS. For me, I think we should keep it, as this a real surface type that appears some of our DUNE, LZ, and XLZD models. Though it is an easy switch so it shouldn't matter either way; Owen and I confirmed that simply commenting out the torus line in SurfaceTypeTraits, the original performance is restored (exactly) in both cases.

[elliottbiondo]

Looks great! It would probably be good to ray trace a torus from a GDML file as a sanity check before merging.

[sethrj]

@elliottbiondo I think it would be good to do a little more analysis on the impact of enabling at runtime before activating it. Options:

• wait to merge before analysis (maybe we could do this friday if we're all here, otherwise it'll have to wait till end of july)
• add a cmake option to disable the runtime torus code (pretty straightforward)
• disable it and then later after analysis do a git revert on the commit

I don't think it's used in any of the DUNE models: just some of the other rare event searches and the LHC detectors?

[elliottbiondo]

@sethrj my vote is add a cmake option and merge this ASAP. @osanstrong can you look into that? Basically without the the cmake flag, the old cylindrical torus approximation would be used and the torus woudln't appear in SurfaceTypeTraits. There would be no runtime impact.

I will have to dig up whatever old DUNE model had a torus. Obviously it's not a key part of the DUNE geometry, but the point is we see torii in production models. When doing performance comparisons with other codes with such models enabling torii isn't a handicap --- it's necessary for a fair comparison.

[sethrj]

One thing is that for our immediate use, even if there is a torus in there, we don't truly need it if it's not in contact with an optical material. I'd rather implement the (possibly much more effective) optimization of moving a subset of the true geometry to the gpu, over always enabling torus.

[elliottbiondo]

Certainly as long as we are principally focused on DUNE it makes sense as a compile-time switch, but as a general-purpose tool, the default should be on.

Since DUNE is so simple, I wouldn't be surprised if commenting out other surface types would bring speedup. The switch statement on surface type is a hotspot in thread divergence, and I could imagine there being a reduction in register usage as well.

[osanstrong]

Toroids should now only be enabled with the preprocessor flag CELERITAS_ENABLE_TOROIDS, e.g. in cacheVariables for a cmake preset:

"cacheVariables": {
    ...
    "CMAKE_CXX_FLAGS": "-Wall -Wextra -pedantic -Wno-stringop-overread -fdiagnostics-color -D CELERITAS_ENABLE_TOROIDS", 
    ...
}

Speaking of which, is there a more preferred way of passing preprocessor flags than this? It had seemed like setting a boolean a la CELERITAS_USE_MPI would have required actively specifying that for every given preset file, but I a) might be wrong b) wouldn't know what's considered "best practice"