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

Allow geometry selection independent of VecGeom being enabled #726

Merged
merged 19 commits into from
Apr 26, 2023

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Apr 19, 2023

This does something similar with geometry that we have for the RNGs: selecting in-house geometry while still building the others. I'm thinking about adding a "simple ORANGE" for performance testing, and it'll be good to have it available without a branch.

This replaces a lot of #if CELERITAS_USE_VECGEOM with #if CELERITAS_GEO == CELERITAS_GEO_VECGEOM, which I think makes it a little clearer that it's about the main geometry selection rather than the availability of the external library.

I also renamed load_gdml to make it clearer that it's loading into the global geant4 state as opposed to VecGeom. The way it also tried to handle memory was also wrong: the GDML reader isn't actually returning a new'd pointer with C++ style memory management; the underlying object saves this to a magical vector that manages memory.

I also reworked the VecGeom test (since it can now be enabled without VecGeom being selected as the "core" geometry) so that it can run all the Geant4 geometry tests without having to break them apart into separate test instances or loading geant physics.

@sethrj sethrj added minor Minor internal changes or fixes external Dependencies and framework-oriented features labels Apr 19, 2023
cmake/CeleritasUtils.cmake Outdated Show resolved Hide resolved
cmake/CeleritasUtils.cmake Outdated Show resolved Hide resolved
cmake/CeleritasUtils.cmake Outdated Show resolved Hide resolved
cmake/CeleritasUtils.cmake Outdated Show resolved Hide resolved
app/CMakeLists.txt Outdated Show resolved Hide resolved
sethrj added 2 commits April 21, 2023 13:39
Provide a helper function that we can use to centralize the
decision/implementation between outputting a message or changing the
function.
@sethrj sethrj requested a review from pcanal April 24, 2023 03:04
@mrguilima mrguilima self-requested a review April 24, 2023 15:59
@sethrj
Copy link
Member Author

sethrj commented Apr 26, 2023

@pcanal Any further comments? I think I've addressed all your concerns. If you want a clearer error message perhaps the following is good enough:

diff --git a/cmake/CeleritasUtils.cmake b/cmake/CeleritasUtils.cmake
index b70f8abc..77d3d304 100644
--- a/cmake/CeleritasUtils.cmake
+++ b/cmake/CeleritasUtils.cmake
@@ -298,8 +298,9 @@ function(celeritas_define_options var doc)

   list(FIND ${var}_OPTIONS "${${var}}" _index)
   if(_index EQUAL -1)
+    string(JOIN "," _optlist ${${var}_OPTIONS})
     celeritas_error_incompatible_option(
-      "Invalid selection not in list: \"${${var}_OPTIONS}\""
+      "Invalid option for ${var}: must be in the list [${_optlist}] "
       "${var}" "${_default}"
     )
   endif()

cmake/CeleritasUtils.cmake Outdated Show resolved Hide resolved
Copy link
Contributor

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

Thanks

@sethrj sethrj merged commit 5679856 into celeritas-project:develop Apr 26, 2023
@sethrj sethrj deleted the geo-choices branch April 26, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Dependencies and framework-oriented features minor Minor internal changes or fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants