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

WIP: remove celeritas #12

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Oct 28, 2024

This improves the usability of g4vg at the cost of removing functionality and duplicating some code from celeritas. Happy to have some help from @agheata and @SeverinDiederichs .

@sethrj sethrj marked this pull request as draft October 28, 2024 11:43
@JuanGonzalezCaminero
Copy link

Hi, because FindCUDA is deprecated (Used by VecCore) the highest CMake version that we can currently use is 3.26

@agheata
Copy link

agheata commented Oct 31, 2024

@sethrj This currently cuts the dependency but does not replace the CELER_XXX macros, so the compilation fails. Are you proposing we take it from here and do the rest of the changes, using a library-local copy of these macros? Just to make sure we don't start doing the same thing on the two sides of the pond. What about the testing macros, should we protect them using some #ifdef guard? We should keep @drbenmorgan in the loop since he also has some branch doing similar stuff in his fork.

@drbenmorgan
Copy link
Contributor

Just for reference, my hacked "standalone" version is on this branch: https://github.com/drbenmorgan/g4vg/commits/standalone/

There are some differences in approach - I went for providing internal workarounds/translations of some of the Celeritas functionality, see https://github.com/drbenmorgan/g4vg/blob/e4b2870b833ac3a9f3d1cb39f579fc6796c5aca4/src/detail/TranslationTypes.hh

I'll have a look at the changes here in more detail.

@drbenmorgan
Copy link
Contributor

Having had a quick look, I suspect that all the remaining build failures are down to the CELER_... macros and other types. As noted above, my branch dealt with this by providing an internal translation:

https://github.com/drbenmorgan/g4vg/blob/e4b2870b833ac3a9f3d1cb39f579fc6796c5aca4/src/detail/TranslationTypes.hh

which I guess is similar to the header in this PR:

https://github.com/celeritas-project/g4vg/pull/12/files#diff-7b2edb38c8fd49855a9f9835ea4cbeb490e3d01d0d5bb1e14a41a02105e85447

That could therefore be expanded to cover the remaining macros/types, or do you have a different plan/ideas here @sethrj?

@sethrj
Copy link
Member Author

sethrj commented Nov 4, 2024

I think that's about right, glad I didn't do any more since I had forgotten about your patch. I paused because I wasn't sure if we wanted to delete, copy, and/or refactor the various features... looks like you got most of it though! A few things:

  • I don't like that we unconditionally remove the assertions since those can check for changes in geant4 behavior etc.
  • We can probably get rid of the type-safe OpaqueId entirely since it's not used for anything other than volume IDs
  • I'd rather copy-paste (and add namespaces to) the softeq/demangle
  • We could find/replace the few uses of for ( _ : range(..)) with for ( ; ; )
  • At the end, let's replace all CELER_ with G4VG_

@drbenmorgan
Copy link
Contributor

Sounds good @sethrj - agreed that the assertions are good to have!

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.

4 participants