Calculate safety distances using OBZ when simple safety is not available #1427
Conversation
|
@sethrj this is ready for your review. I believe everything in this MR and my previous MR are correct, but I am suspicious of the OBZ inner bbox that is being created upon construction. In the GDML file I added here, I have a cone of r=13 and z=50 centered at the origin. As you can see, the outer bbox is correct. However, I don't under why the inner bbox spans z = (-12.5, 0). No matter what is chosen for the inner bbox x and y widths, the inner bbox should always start at z=-25. Obviously the way we are calculating bboxs by truncating all space with planes does not always yield the analytic solution, but I don't see where the -12.5 could be coming from. OBZ for the cone volume |
Test summary 3 157 files 4 968 suites 3m 21s ⏱️ For more details on these failures, see this check. Results for commit 54e5cef. ♻️ This comment has been updated with latest results. |
elliottbiondo
force-pushed
the
safety
branch
from
e8c35d4 to
7e213ec
Compare
|
CI failures from |
sethrj
added
enhancement
| @@ -100,6 +101,7 @@ class SimpleUnitTracker | |||
|
|
|||
| ParamsRef const& params_; | |||
| SimpleUnitRecord const& unit_record_; | |||
| detail::OrientedBoundingZone::StoragePointers obz_storage_; | |||
There was a problem hiding this comment.
This should probably be created on the fly since we keep params_ around
| = params_.volume_records[unit_record_.volumes[volid]].obz_id; | ||
| detail::OrientedBoundingZone obz(params_.obz_records[obz_id], | ||
| obz_storage_); | ||
| return obz.calc_safety_inside(pos); |
There was a problem hiding this comment.
Unfortunately it's not going to be this simple: we need the minimum over both "inside current volume" and "outside all directly enclosed volumes". I'm not sure if we have that metadata yet, in fact...
| <physvol> | ||
| <volumeref ref="vol3"/> | ||
| <position ref="origin"/> | ||
| </physvol> |
There was a problem hiding this comment.
I think these three enclosed volumes are overlapping. It might be more natural for you to make a geometry with orange/orangeinp/UnitProto.test.cc using the InputBuilderTest harness.
| @@ -967,6 +973,22 @@ TEST_F(FiveVolumesTest, TEST_IF_CELERITAS_DOUBLE(heuristic_init)) | |||
| } | |||
| } | |||
|
|
|||
| TEST_F(ConeTest, safety) | |||
| { | |||
There was a problem hiding this comment.
Oh crap, I forgot that the Geant input gets scaled into the native unit system (unlike the .org.json files which are precalculated). This test is with the CLHEP unit system which scales everything by a factor of 10. I think it would be better to build a geometry with the InputBuilder test... sorry 😞
|
There's also a build error due to |
|
Closing this MR. Discussing with @sethrj, this approach will not work in the short term. The OBZ inner bounding box does not exclude inner volumes (this is a good thing because if it did, it would be too small to be useful in many cases). A consequence of this is that when checking inner safety distances, we would have to check the outer safety distances of internal volumes. Volumes don't currently have metadata denoting what internal volumes are contained. While this could be remedied when converting from G4 (in which the concept of internal volumes is a baked into the geometry format), it is not clear how it could be done during construction in the general CSG case. As we implement more specialized features we may need to split things up so that ORANGE differentiates between HEP and nuclear (i.e., general CSG) geometries, which would be a considerable undertaking. |
The MR "hooks up" the OBZ safety distance calculation capability by using it within
SimpleUnitTrackerto calculate safety distances when simple safety is not available.