Navigation refactoring moving navigators in VecGeom #264
Conversation
* Adapted the BVHNavigator to work with the modified BVH in Vecgeom * Increased device stack limit in example17 * Move BVH and Loop navigators to VecGeom
* Take into account that older versions do not provide VecGeom_VERSION_STRING
|
Can one of the admins verify this patch? |
|
|
||
| } // End namespace COPCORE_IMPL | ||
| #endif // RT_LOOP_NAVIGATOR_H_ | ||
| #endif // RT_LOOP_NAVIGATOR_H_ |
There was a problem hiding this comment.
RT_NAVIGATOR_H_
There was a problem hiding this comment.
OK we fix this in a separate PR not to re-trigger the CI now
|
Hi, I ran my usual tests before and after this PR, also switching VecGeom from On AdePT commit eb811d9 (before this PR), comparing Then updating AdePT and VecGeom to the latest For performance, I see a quite measurable time difference for both migrations with stronger magnetic fields ( |
|
The crashes in examples when using cms_2018 are likely due to not increasing the stack limit, after a commit in VecGeom that was setting it to 8KB by default. Many examples are already protected, none is crashing on my machine or on the test machine, but we probably need to extend the protection. About the reproducibility and performance measurements, @JuanGonzalezCaminero can comment. |
|
Hi Jonas, indeed the results are no longer numerically identical, the comparison was done before the commit moving the navigators to VecGeom. |
This is the fix for extruded solids non-symmetrical about XoY |
Ok, but there are two sources of differences that should be understood individually:
This commit touches PolygonalShell which is only used by simple extruded solids. There are no extruded solids in
Ok, I picked only that one commit and the BVH refactoring alone seems to give numerical results while staying with VecGeom commit https://gitlab.cern.ch/VecGeom/VecGeom/-/commit/44ec0798fcc7d689246fb214620e520382d6c294 Can run more tests next week, if needed. |
|
The only source of differences is actually coming from this merge request: https://gitlab.cern.ch/VecGeom/VecGeom/-/merge_requests/1004 (commit e607cbc72ac323c31747c0181c4e1ead886965ea) introducing operator*= for Transformation3D and applying it for cached transformations. This is one commit after the one fixing the simple extruded. The rationale was twofold: having numerical matching between transformations as computed in Geant4, and doing less copies while getting the top transformation in the non-cached mode. Basically, we replace the weird operation MultiplyFromRight with a normal matrix multiplication like in G4. However, due to the different order of operations, truncation is done differently so the final matrices and not numerically equal. There is a unit test that was implemented at the same time, and replacing the The move to master before the refactoring is probably an effect of my starting from Juan's branch, and adding the version change on top of it, I was rushing because the CI was broken by a premature VecGeom tag deployment. |
Alright, this explains the differences seen together with the refactoring of the BVH navigator and the move from AdePT to VecGeom. The difference between |
It's https://gitlab.cern.ch/VecGeom/VecGeom/-/commit/011010a2 which makes sense in retrospective, I guess... (edit: and https://gitlab.cern.ch/VecGeom/VecGeom/-/commit/380a0d48 as well; but I still can't exactly reproduce the results of |
Ah, OK, yes, it is understandable now. Can we run the testEM3 and example17 validation vs. Geant4 to make sure that all these transformation changes (tested to alter the values of transformation elements at ~1E-15 level) do not affect physics validation? |
This combines the VecGeom version change and the draft BVH refactoring, to have a coherent migration in one go:
Moving the requested version of VecGeom to 2.0.0-dev.3, which contains the navigators moved from AdePT, and needed by Draft BVH Refactoring #261. We need to use VecGeom_VERSION_STRING to select dev versions of VecGeom until the 2.0.0 release
Adapting the BVHNavigator to work with the modified BVH in VecGeom (by @JuanGonzalezCaminero)
Validation done:
Performance comparison: