-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix using ORANGE geometry with pointer-appended GDML #960
Conversation
What was the reason for regenerating the GDML? |
@amandalund I'm finally running a benchmark problem with simple CMS divided into r,z,theta to see how the scaling works. The geometry exporter by @stognini saves pointers (and it's reasonable to assume other user inputs will do the same). I regenerated the testem3 gdml only inside the test directory in order to check that leaving the pointers still works with GDML+ORANGE. |
if (std::distance(start, stop) == 1) | ||
{ | ||
return start->second.second; | ||
} | ||
|
||
// Multiple labels match | ||
CELER_LOG(warning) | ||
<< "Multiple materials match the volume '" << vol_label << "': " | ||
<< join_stream( | ||
start, stop, ", ", [](std::ostream& os, auto&& mliter) { | ||
os << mliter.second.second.unchecked_get(); | ||
}); | ||
return start->second.second; |
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.
Maybe I'm reading this wrong, but since from this point on we return start->second.second
for the MaterialId
, might be easier to read if we simply do:
if (std::distance(start, stop) > 1)
{
// Multiple labels match
CELER_LOG(warning)
<< "Multiple materials match the volume '" << vol_label << "': "
<< join_stream(
start, stop, ", ", [](std::ostream& os, auto&& mliter) {
os << mliter.second.second.unchecked_get();
});
}
return start->second.second;
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.
Good point. Thanks! With the output @amandalund got though, it'll make even more sense to build a temporary 'set' and only print if more than one ID exists in the set.
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.
Thanks @sethrj, I think the logic is easier to follow now. I tried loading the cms2018, cms-run3, and cms-hllhc geometries through celer-sim and celer-g4. I didn't notice any changes with celer-g4; with celer-sim the warnings are slightly different, but as long as that's expected I think this looks good.
cms2018 has the additional warnings:
/home/alund/celeritas_project/celeritas/src/celeritas/geo/GeoMaterialParams.cc:125: warning: Multiple materials match the volume 'PixelForwardHoldersConnector@0x7f4a8f51d600': 277, 277, 277, 277
/home/alund/celeritas_project/celeritas/src/celeritas/geo/GeoMaterialParams.cc:125: warning: Multiple materials match the volume 'PixelForwardHoldersConnector@0x7f4a8f4d5840': 277, 277, 277, 277
/home/alund/celeritas_project/celeritas/src/celeritas/geo/GeoMaterialParams.cc:125: warning: Multiple materials match the volume 'PixelForwardBladeBase@0x7f4a8f4cb7c0': 277, 277, 277, 277
/home/alund/celeritas_project/celeritas/src/celeritas/geo/GeoMaterialParams.cc:125: warning: Multiple materials match the volume 'PixelForwardModule@0x7f4a9a7ccb10': 277, 277, 277, 277
/home/alund/celeritas_project/celeritas/src/celeritas/geo/GeoMaterialParams.cc:125: warning: Multiple materials match the volume 'PixelForwardHoldersConnector@0x7f4a8f4d5a80': 277, 277, 277, 277
...
/home/alund/celeritas_project/celeritas/src/celeritas/geo/GeoMaterialParams.cc:125: warning: Multiple materials match the volume 'PixelForwardModule@0x7f4a9a7cfcf0': 277, 277, 277, 277
and all three geometries had fewer volumes without known material IDs.
I think all the warnings @amandalund showed should be gone. It'll only warn if materials are different. |
I created a new testem3 GDML file with @stognini 's generator, and updated the equivalent ORANGE file, but it failed to run:
This is because the geo-material mapper only considered the options that either the name+extension match exactly (e.g. when loading with vecgeom gdml + vecgeom geometry), or when only the name matches (can happen when using geant with pointer stripping?), but it didn't allow for the case when loading a GDML through geant4 for physics but using the ORANGE geometry for tracking.
I've refactored the geomaterial code to do a better job of mapping volume names. Hopefully the code is clearer, and I've updated one of the tests to ensure it still works. Before merging I'd like to make sure that both the
celer-sim
andaccel
pathways work with geometries that have duplicate names (but uniquifying extensions) like in cms.