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: introduce material accumulation interface (MM3) #3020

Merged
merged 14 commits into from
Apr 8, 2024

Conversation

asalzburger
Copy link
Contributor

This is the first PR in a series that divides the Material Mapping into logical, unit testable modules:

  1. Finding intersections with surfaces and associations to volumes (MM2)
  2. Assigning material interactions to those intersections (MM1)
  3. Mapping those onto dedicated Surface / Volume Material Mappers (MM3, this PR, partly)
  4. Steer that by a chained algorithm (MM4)

This PR:

It encapsulates the pure accumulation of the material for Surface based materials into a new module. The accumulation is now decoupled from the association, i.e. one can run the mapping with a navigator or with a trial and error method, and still use the exact same material mapper.
The code is largely transferred from the 'SurfaceMaterialMapper' which will vanish to exist after this re-organization.

@Corentin-Allaire - the State of this Mapper would have all the access to run the optimisation, I have not yet re-introduced the track variance, but that should be a five-line change.

This PR is blocked by #3015 and #3016.

All cases are showcased and tested in a set of UnitTests.

@asalzburger asalzburger added this to the next milestone Mar 11, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Mar 11, 2024
@asalzburger asalzburger added the 🛑 blocked This item is blocked by another item label Mar 11, 2024
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 53.12500% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 49.13%. Comparing base (1374baa) to head (df7774a).
Report is 1 commits behind head on main.

❗ Current head df7774a differs from pull request most recent head bcf5f36. Consider uploading reports for the commit bcf5f36 to get more accurate results

Files Patch % Lines
.../src/Material/BinnedSurfaceMaterialAccumulater.cpp 48.64% 4 Missing and 34 partials ⚠️
...ore/src/Material/MaterialInteractionAssignment.cpp 50.00% 0 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3020      +/-   ##
==========================================
+ Coverage   49.02%   49.13%   +0.10%     
==========================================
  Files         494      498       +4     
  Lines       29058    29077      +19     
  Branches    13797    13795       -2     
==========================================
+ Hits        14246    14287      +41     
+ Misses       4929     4905      -24     
- Partials     9883     9885       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Corentin-Allaire Corentin-Allaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few comments

Core/include/Acts/Material/MaterialInteraction.hpp Outdated Show resolved Hide resolved
Core/src/Material/BinnedSurfaceMaterialAccumulater.cpp Outdated Show resolved Hide resolved
@asalzburger asalzburger removed the 🛑 blocked This item is blocked by another item label Apr 8, 2024
@asalzburger
Copy link
Contributor Author

Comments should be addressed now.

Copy link
Contributor

@Corentin-Allaire Corentin-Allaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks ! I will have a look at MM4 later today

@asalzburger asalzburger merged commit 6a36ee4 into acts-project:main Apr 8, 2024
51 checks passed
@asalzburger asalzburger deleted the feat-mm3-accumulation branch April 8, 2024 13:14
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Apr 8, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
)

This is the first PR in a series that divides the Material Mapping into
logical, unit testable modules:

1) Finding intersections with surfaces and associations to volumes (MM2)
2) Assigning material interactions to those intersections (MM1)
3) Mapping those onto dedicated Surface / Volume Material Mappers (MM3,
this PR, partly)
4) Steer that by a chained algorithm (MM4)

This PR:

It encapsulates the pure accumulation of the material for Surface based
materials into a new module. The accumulation is now decoupled from the
association, i.e. one can run the mapping with a navigator or with a
trial and error method, and still use the exact same material mapper.
The code is largely transferred from the 'SurfaceMaterialMapper' which
will vanish to exist after this re-organization.

@Corentin-Allaire - the State of this Mapper would have all the access
to run the optimisation, I have not yet re-introduced the track
variance, but that should be a five-line change.

This PR is blocked by acts-project#3015 and acts-project#3016.


All cases are showcased and tested in a set of UnitTests.
@andiwand andiwand modified the milestones: next, v34.1.0 Apr 25, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
)

This is the first PR in a series that divides the Material Mapping into
logical, unit testable modules:

1) Finding intersections with surfaces and associations to volumes (MM2)
2) Assigning material interactions to those intersections (MM1)
3) Mapping those onto dedicated Surface / Volume Material Mappers (MM3,
this PR, partly)
4) Steer that by a chained algorithm (MM4)

This PR:

It encapsulates the pure accumulation of the material for Surface based
materials into a new module. The accumulation is now decoupled from the
association, i.e. one can run the mapping with a navigator or with a
trial and error method, and still use the exact same material mapper.
The code is largely transferred from the 'SurfaceMaterialMapper' which
will vanish to exist after this re-organization.

@Corentin-Allaire - the State of this Mapper would have all the access
to run the optimisation, I have not yet re-introduced the track
variance, but that should be a five-line change.

This PR is blocked by acts-project#3015 and acts-project#3016.


All cases are showcased and tested in a set of UnitTests.
asalzburger added a commit to asalzburger/acts that referenced this pull request May 21, 2024
)

This is the first PR in a series that divides the Material Mapping into
logical, unit testable modules:

1) Finding intersections with surfaces and associations to volumes (MM2)
2) Assigning material interactions to those intersections (MM1)
3) Mapping those onto dedicated Surface / Volume Material Mappers (MM3,
this PR, partly)
4) Steer that by a chained algorithm (MM4)

This PR:

It encapsulates the pure accumulation of the material for Surface based
materials into a new module. The accumulation is now decoupled from the
association, i.e. one can run the mapping with a navigator or with a
trial and error method, and still use the exact same material mapper.
The code is largely transferred from the 'SurfaceMaterialMapper' which
will vanish to exist after this re-organization.

@Corentin-Allaire - the State of this Mapper would have all the access
to run the optimisation, I have not yet re-introduced the track
variance, but that should be a five-line change.

This PR is blocked by acts-project#3015 and acts-project#3016.


All cases are showcased and tested in a set of UnitTests.
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.

None yet

4 participants