Skip to content
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

feat: Delayed Grid construction for Portals #3718

Merged
merged 10 commits into from
Oct 11, 2024

Conversation

paulgessinger
Copy link
Member

This PR does three things:

  • PortalLinkBase::merge no longer does merging of grids or trivials into a combined grid. This has been observed to lead to accumulating floating point imprecision.
  • CompositePortalLink gets a method to make a grid if (and only if) it is composed of a set of TrivialPortalLinks.
  • Portal::fuse will attempt to convert CompositePortalLinks composed of only TrivialPortalLinks to a single GridPortalLink before fusing it with the other portal.

In combination, this avoids the floating point issues and produces correctly sized grids.

Part of #3502.


This pull request introduces several enhancements and bug fixes to the Core/include/Acts/Geometry module, focusing on improving the CompositePortalLink and GridPortalLink classes. The most important changes include the addition of new constructors, the introduction of the makeGrid method, and various adjustments to ensure compatibility and correctness.

Enhancements to CompositePortalLink:

  • Added new constructors to allow the creation of composite portals from multiple links and to handle nested composites. (Core/include/Acts/Geometry/CompositePortalLink.hpp, [1] [2]
  • Introduced the makeGrid method to convert composite portal links into grid portal links under specific conditions. (Core/include/Acts/Geometry/CompositePortalLink.hpp, Core/src/Geometry/CompositePortalLink.cppR180-R297)

Adjustments to GridPortalLink:

Bug Fixes and Code Improvements:

  • Moved mergedSurface function to an anonymous namespace in the implementation file and refactored it for better error handling and type safety. (Core/src/Geometry/CompositePortalLink.cpp, [1] [2]
  • Improved the isSameSurface function to include detailed checks for surface bounds and transformations. (Core/src/Geometry/Portal.cpp, Core/src/Geometry/Portal.cppL308-R357)
  • Enhanced logging and error messages for better debugging and clarity. (Core/src/Geometry/Portal.cpp, [1] [2]

These changes collectively improve the functionality, safety, and maintainability of the Acts geometry module, particularly in handling complex portal link structures.

@paulgessinger paulgessinger added this to the next milestone Oct 11, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Oct 11, 2024
@paulgessinger paulgessinger mentioned this pull request Oct 11, 2024
@paulgessinger paulgessinger changed the title feat: Delayed Grid construction feat: Delayed Grid construction for Portals Oct 11, 2024
Copy link

github-actions bot commented Oct 11, 2024

📊: Physics performance monitoring for c64751b

Full contents

physmon summary

Copy link

sonarcloud bot commented Oct 11, 2024

@kodiakhq kodiakhq bot merged commit 9bd074c into acts-project:main Oct 11, 2024
42 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 11, 2024
@paulgessinger paulgessinger modified the milestones: next, v37.1.0 Oct 16, 2024
Rosie-Hasan pushed a commit to Rosie-Hasan/acts that referenced this pull request Nov 13, 2024
This PR does three things:

- `PortalLinkBase::merge` **no longer** does merging of grids or trivials into a combined grid. This has been observed to lead to accumulating floating point imprecision.
- `CompositePortalLink` gets a method to make a grid **if** (and only if) it is composed of a set of `TrivialPortalLink`s.
- `Portal::fuse` will attempt to convert `CompositePortalLink`s composed of only `TrivialPortalLink`s to a single `GridPortalLink` before fusing it with the other portal.

In combination, this avoids the floating point issues and produces correctly sized grids.

Part of acts-project#3502.

---

This pull request introduces several enhancements and bug fixes to the `Core/include/Acts/Geometry` module, focusing on improving the `CompositePortalLink` and `GridPortalLink` classes. The most important changes include the addition of new constructors, the introduction of the `makeGrid` method, and various adjustments to ensure compatibility and correctness.

### Enhancements to `CompositePortalLink`:

* Added new constructors to allow the creation of composite portals from multiple links and to handle nested composites. (`Core/include/Acts/Geometry/CompositePortalLink.hpp`, [[1]](diffhunk://#diff-248145d68399a17324b82d81d6e661a3ab739eb5b6f8d67f40145195ca465c36R55-R63) [[2]](diffhunk://#diff-5663ec0ea1d9723e610725aa0d0964e5c833bb90f431281c3802a9b9c5fa4314R101-R129)
* Introduced the `makeGrid` method to convert composite portal links into grid portal links under specific conditions. (`Core/include/Acts/Geometry/CompositePortalLink.hpp`, [Core/src/Geometry/CompositePortalLink.cppR180-R297](diffhunk://#diff-5663ec0ea1d9723e610725aa0d0964e5c833bb90f431281c3802a9b9c5fa4314R180-R297))

### Adjustments to `GridPortalLink`:

* Changed the type of `atLocalBins` methods to return `const TrackingVolume*` instead of `TrackingVolume*`. (`Core/include/Acts/Geometry/GridPortalLink.hpp`, [[1]](diffhunk://#diff-5cc33f33e4da7753510a3c7bf2481d12c34dd9c1344bfd624c0b5db1d70f214fL384-R389) [[2]](diffhunk://#diff-5cc33f33e4da7753510a3c7bf2481d12c34dd9c1344bfd624c0b5db1d70f214fL402-R402) [[3]](diffhunk://#diff-5cc33f33e4da7753510a3c7bf2481d12c34dd9c1344bfd624c0b5db1d70f214fL548-R547) [[4]](diffhunk://#diff-5cc33f33e4da7753510a3c7bf2481d12c34dd9c1344bfd624c0b5db1d70f214fL560-R559)
* Updated internal grid type to use `const TrackingVolume*`. (`Core/include/Acts/Geometry/GridPortalLink.hpp`, [Core/include/Acts/Geometry/GridPortalLink.hppL402-R402](diffhunk://#diff-5cc33f33e4da7753510a3c7bf2481d12c34dd9c1344bfd624c0b5db1d70f214fL402-R402))

### Bug Fixes and Code Improvements:

* Moved `mergedSurface` function to an anonymous namespace in the implementation file and refactored it for better error handling and type safety. (`Core/src/Geometry/CompositePortalLink.cpp`, [[1]](diffhunk://#diff-5663ec0ea1d9723e610725aa0d0964e5c833bb90f431281c3802a9b9c5fa4314R11-R78) [[2]](diffhunk://#diff-5663ec0ea1d9723e610725aa0d0964e5c833bb90f431281c3802a9b9c5fa4314L77-L106)
* Improved the `isSameSurface` function to include detailed checks for surface bounds and transformations. (`Core/src/Geometry/Portal.cpp`, [Core/src/Geometry/Portal.cppL308-R357](diffhunk://#diff-e32791625fda93fd367fc971619ea03be19128e91bbca7e8a09b5af399beb461L308-R357))
* Enhanced logging and error messages for better debugging and clarity. (`Core/src/Geometry/Portal.cpp`, [[1]](diffhunk://#diff-e32791625fda93fd367fc971619ea03be19128e91bbca7e8a09b5af399beb461R274-R277) [[2]](diffhunk://#diff-e32791625fda93fd367fc971619ea03be19128e91bbca7e8a09b5af399beb461R293-R313)

These changes collectively improve the functionality, safety, and maintainability of the `Acts` geometry module, particularly in handling complex portal link structures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants