-
Notifications
You must be signed in to change notification settings - Fork 32
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
Convert Geant4 geometry to VecGeom in memory #557
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice @mrguilima ! I've left a first round of comments.
FYI the CI is failing, possibly because of a version difference between the VecGeom versions?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, there is still plenty that can be improved, but since the plan is to move this to a separate library (and because it's derivative code) let's just call it good. I'm going to add to the code list to LICENSE before merging.
This is with: vecgeom@1.2.2~cuda+gdml~geant4~ipo~root+shared build_system=cmake build_type=RelWithDebInfo cxxstd=17 generator=make %apple-clang@14.0.0 arch= darwin-ventura-m2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrguilima I built your branch and spent some detailed time looking it over last night and fixing issues.
- It didn't build on my mac at first (because vecgeom doesn't define DeviceVec if CUDA isn't enabled)
- I didn't catch that these detail classes weren't in the detail namespace
- By removing geant4 includes from the header I'm able to directly include the file from VecgeomParams
- Several functions were unused and several unused macro-blocked pieces of code had errors in them; I deleted both
- Temporarily changed the test results to pass on my mac: particles are getting stuck in the torus
The last one is particularly worrisome because it means we're getting different results from different builds of VecGeom. Since the torus tracking fails both in direct vecgeom and on the G4 constructed one, perhaps it's better to punt on that, remove the torus from our unit test, and report a bug on vecgeom jira?
Three other big things:
- I think the converter is leaking a ton of memory. It's unclear from the code which of the bajillion pointers take ownership.
- Constructing boolean volumes (addition, subtraction) results in duplicate name warnings like
warning: Geometry contains duplicate volume names: "placedB_bool_right" (10), "placedB_bool_right" (12)
. These duplicate names are all over the place in the CMS run 3 geometry (even with VGDML) and seem to break our ability to transport on it. Can you figure out why these are being constructed and how? Is one a temporary? - It looks like the world pointer that we pass to
geo_manager.SetWorld
get copied instead of owned, so we won't be able to play any tricks with the pointer maps we build so the vecgeom pointers are useless. We'll need to build a separate map ofg4_logical_volume.GetInstanceID()
tovecgeom_logical_volume.id()
.
This reverts commit e220a9d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit e220a9d breaks my VecGeom tests in Linux (as that commit message implies).
Following up from @mrguilima 's tests: building vecgeom (can you confirm it's 1.2.2?) with:
the tests pass without the |
Once the CI passes I'm going to merge! 🎉 |
This is a first "working" version of the Geant4-to-VecGeom in-memory geometry conversion, for a first round of comments.
Known issues:
G4PhysicalVolumeStore::GetInstance()->Clean()
results into a crash at the second testSetup()
Setup()
onwards