fix: DetectorVolume containment check #2895
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Component - Core
|
Forgot to add. The number of segments for polyhedronRepresentation is hardcoded to be 1k. The default option of 1 didn't work with cylindrical shapes on more complex side -- the Extent that was returned for what I think was a barrel/endcap had big enough errors in phi/radius for the check to fail |
|
Moved the check inside the DetectorVolumeFactory::construct(). The one that takes the surfaces and subvolumes. Also, made the number of polyhedron segments a parameter with a default value |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2895 +/- ##
==========================================
- Coverage 48.84% 48.84% -0.01%
==========================================
Files 492 492
Lines 28846 28861 +15
Branches 13677 13685 +8
==========================================
+ Hits 14091 14096 +5
- Misses 4949 4955 +6
- Partials 9806 9810 +4 ☔ View full report in Codecov by Sentry. |
|
Made minor adjustments to the geometry inside the DD4hep Cylindrical Detector test |
…gBox implementation to the parent class
📊: Physics performance monitoring for e147aa2physmon summary
|
asalzburger
previously approved these changes
Jan 25, 2024
|
@asalzburger, looks like it is still waiting for your approval |
|
@asalzburger, Just a reminder that the policy-bot is still waiting for the approval |
asalzburger
approved these changes
Mar 6, 2024
|
we need a codecov overwrite here @asalzburger @paulgessinger |
asalzburger
added
automerge
and removed
Component - Core
dimitra97
pushed a commit
to dimitra97/acts
that referenced
this pull request
Mar 19, 2024
The PR makes changes to the DetectorVolume code that address the issue of the surfaces/subvolumes containment checks, i.e. making sure that they are fully inside the volume. Additionally, minor fixes are made in the Json-related parts of the code (see below). DetectorVolume checkContainment call is moved from the constructor to the end of the "construct" method, where the volume's portals are initialized. Because the containment checks rely on the volume having the initialized Extent and the Extent is derived from the volume's portals, keeping the call in the constructor prevents the proper check from happening, as there are yet no portals to derive the Extent from. Additionally, assert is replaced with invalid_argument thrown in case the internals are not contained. To check the compatibility of the external and internal extents a new "canonicalBinning" method is introduced to the VolumeBounds. It returns the binning values that should be enough to describe the relevant parts of the Extent for a given volume shape, e.g. X,Y,Z bins are returned for cubic volumes, as, for example, there is no reason to check the binMag. Some of the VolumeBounds return the bounding box binning for now as a placeholder. A UnitTest is added to the DetectorVolumeTests that checks incompatibility with internals that are not contained. A minor fixes had to be introduced to DetectorJsonConverter with respect to the geometry parameters. In Transform3 Json writer the default option "transpose" is changed to be false to be consistent with the default reading options.
EleniXoch
pushed a commit
to EleniXoch/acts
that referenced
this pull request
May 6, 2024
The PR makes changes to the DetectorVolume code that address the issue of the surfaces/subvolumes containment checks, i.e. making sure that they are fully inside the volume. Additionally, minor fixes are made in the Json-related parts of the code (see below). DetectorVolume checkContainment call is moved from the constructor to the end of the "construct" method, where the volume's portals are initialized. Because the containment checks rely on the volume having the initialized Extent and the Extent is derived from the volume's portals, keeping the call in the constructor prevents the proper check from happening, as there are yet no portals to derive the Extent from. Additionally, assert is replaced with invalid_argument thrown in case the internals are not contained. To check the compatibility of the external and internal extents a new "canonicalBinning" method is introduced to the VolumeBounds. It returns the binning values that should be enough to describe the relevant parts of the Extent for a given volume shape, e.g. X,Y,Z bins are returned for cubic volumes, as, for example, there is no reason to check the binMag. Some of the VolumeBounds return the bounding box binning for now as a placeholder. A UnitTest is added to the DetectorVolumeTests that checks incompatibility with internals that are not contained. A minor fixes had to be introduced to DetectorJsonConverter with respect to the geometry parameters. In Transform3 Json writer the default option "transpose" is changed to be false to be consistent with the default reading options.
asalzburger
pushed a commit
to asalzburger/acts
that referenced
this pull request
May 21, 2024
The PR makes changes to the DetectorVolume code that address the issue of the surfaces/subvolumes containment checks, i.e. making sure that they are fully inside the volume. Additionally, minor fixes are made in the Json-related parts of the code (see below). DetectorVolume checkContainment call is moved from the constructor to the end of the "construct" method, where the volume's portals are initialized. Because the containment checks rely on the volume having the initialized Extent and the Extent is derived from the volume's portals, keeping the call in the constructor prevents the proper check from happening, as there are yet no portals to derive the Extent from. Additionally, assert is replaced with invalid_argument thrown in case the internals are not contained. To check the compatibility of the external and internal extents a new "canonicalBinning" method is introduced to the VolumeBounds. It returns the binning values that should be enough to describe the relevant parts of the Extent for a given volume shape, e.g. X,Y,Z bins are returned for cubic volumes, as, for example, there is no reason to check the binMag. Some of the VolumeBounds return the bounding box binning for now as a placeholder. A UnitTest is added to the DetectorVolumeTests that checks incompatibility with internals that are not contained. A minor fixes had to be introduced to DetectorJsonConverter with respect to the geometry parameters. In Transform3 Json writer the default option "transpose" is changed to be false to be consistent with the default reading options.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR makes changes to the DetectorVolume code that address the issue of the surfaces/subvolumes containment checks, i.e. making sure that they are fully inside the volume. Additionally, minor fixes are made in the Json-related parts of the code (see below).
DetectorVolume checkContainment call is moved from the constructor to the end of the "construct" method, where the volume's portals are initialized. Because the containment checks rely on the volume having the initialized Extent and the Extent is derived from the volume's portals, keeping the call in the constructor prevents the proper check from happening, as there are yet no portals to derive the Extent from. Additionally, assert is replaced with invalid_argument thrown in case the internals are not contained.
To check the compatibility of the external and internal extents a new "canonicalBinning" method is introduced to the VolumeBounds. It returns the binning values that should be enough to describe the relevant parts of the Extent for a given volume shape, e.g. X,Y,Z bins are returned for cubic volumes, as, for example, there is no reason to check the binMag. Some of the VolumeBounds return the bounding box binning for now as a placeholder.
A UnitTest is added to the DetectorVolumeTests that checks incompatibility with internals that are not contained.
A minor fixes had to be introduced to DetectorJsonConverter with respect to the geometry parameters.
In Transform3 Json writer the default option "transpose" is changed to be false to be consistent with the default reading options.