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

Integration of the VecGeom surface model in the refactored AdePT library #298

Open
agheata opened this issue Aug 6, 2024 · 4 comments
Open

Comments

@agheata
Copy link
Contributor

agheata commented Aug 6, 2024

The VecGeom surface model is currently in development, but much more mature than when it was integrated into the example-based AdePt. We need to integrate the current version into the library-based AdePT similarly:

  • Support the solid and surface models simultaneously in the first phase.
    • Add an updated surface navigator class, following the AdePT navigation interface
    • Use the cmake define ADEPT_USE_SURF as a compile-time switch between the solid/surface models

Issues:

  • Currently the conversion in memory of Geant4 geometry to VecGeom using g4vg is not working with the surface model, because it pulls in a non-working surface model interface from Celeritas. This reveals potential compilation problems of AdePT whenever backward-incompatible changes are made in the surface model, and calls for making the g4vg library independent of Celeritas infrastructure.
@sethrj
Copy link
Contributor

sethrj commented Sep 18, 2024

@agheata One thing that would help this compatibility issue is to move components "upstream" from Celeritas/AdePT into VecGeom: I think some of the compatibility errors are due to us having to copy and modify the surface navigator from AdePT. Do you think VecGeom itself would be a good place for the navigator to live?

@agheata
Copy link
Contributor Author

agheata commented Sep 18, 2024

@sethrj Thanks for bringing this up, I am pretty sure we already moved upstream from AdePT to VecGeom everything that was movable. The surface navigator always lived in VecGeom, and what we have in AdePT is just a tiny layer required because the interfaces are not 100% compatible with the solid model ones (which were also moved to VecGeom), and the conversion + copy to GPU goes in a different way. But this tiny layer in AdePT never interfered with Celeritas compilation, and you could have implemented with little effort the navigation with surfaces using directly the VecGeom interface with no copy from AdePT.

The problem is not that at all, and given the fact that the interface to the surface navigation is not yet stable, even maintaining in Celeritas the interface to an older surface model version would never pose a problem. The problem is in the way the G4VG converter is not pushed upstream to be independent of Celeritas and exposes way too many of its internals, even though it does not need to. Why does the compilation of G4VG in AdePT have to compile the Celeritas interface to VecGeom navigation?? I totally understand that G4VG was rewritten as a Celeritas component to start with, but then when moving it upstream you actually never decoupled the dependencies, making this library unusable for AdePT in the short term, and also making the compilation of AdePT as expensive as AdePT + Celeritas altogether.

@drbenmorgan offered to look into this, but IMHO it would be the easiest for you to make this dependency separation change since you are the most familiar with the conversion code. This would prevent us from doing unnecessary copy/reimplementation of G4VG, while we already have our hands full with developing a surface model that will likely be used by both projects.

@sethrj
Copy link
Contributor

sethrj commented Sep 18, 2024

@agheata That's great to know. I do remember the BVH navigator being moved (I forgot that our copy is only there for compatibility with older vecgeom versions) and will check with Guilherme about the current navigation and setup.

I realize we've had this discussion before and never came to a satisfactory conclusion. You're right that most of the Celeritas infrastructure for the converter itself is unnecessary: there are four or five diagnostic classes which could be replaced by the vecgeom logger and/or moved to a higher level, and then the code could be moved to an intermediate library that depends solely on Geant4 and VecGeom.

However, the main reason I kept the Celeritas geometry wrappers in place, and the main reason this will be hard to decouple, is testing. We've invested heavily in the testing infrastructure in Celeritas, and it has helped us catch numerous bugs, assumptions, and missing features in ORANGE, VecGeom, and Geant4 navigators. The code for that testing framework is substantial and would require a lot of duplication or backporting or exporting another "core" layer for testing in G4VG.

Here's a potential compromise. I could refactor the conversion library into a project independent from Celeritas, but for testing it will set up an "external" Celeritas and rely on its testing framework for the conversion. That way only the CI and developers who work on the conversion layer will have to use those components. Shall we try to implement this over the week together in October?

Remember also that our hands are also quite full: between the growing application space for Celeritas, and personnel availability issues, we're already heavily oversubscribed. But we do very much value this collaboration, and an efficient surface model (with Geant4 conversion) will benefit us both greatly, so we will continue to invest in VecGeom. I hope to find a solution that satisfies us both, and in addition find the time to implement it.

@agheata
Copy link
Contributor Author

agheata commented Sep 19, 2024

@sethrj this sounds like a good compromise. In VecGeom the testing layer also depends on ROOT/Geant4, which is acceptable if it's only enabled for dev/CI. Sure, we can work on this in October.

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

No branches or pull requests

2 participants