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: encapsulate assignment of material interactions to material surfaces (MM1) #3015

Merged

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, this PR)
  3. Mapping those onto dedicated Surface / Volume Material Mappers (MM3)
  4. Steer that by a chained algorithm (MM4)

This PR:

The logic how to walk through material interactions and assign those is currently implemented in both the SurfaceMaterialMapper and VolumeMaterialMapper and consists of many If/else cases that are rather difficult to follow.
This PR (after consultation with @Corentin-Allaire ) introduces a single logic for Surface based material that allows to define global and local vetos (global, e.g. for volume restrictions) and re-assignment policies for pre/post mapping directives.

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

@asalzburger asalzburger added this to the next milestone Mar 8, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

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

Project coverage is 48.99%. Comparing base (2b51bcb) to head (d120036).

Files Patch % Lines
...ore/src/Material/MaterialInteractionAssignment.cpp 51.02% 0 Missing and 24 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3015   +/-   ##
=======================================
  Coverage   48.98%   48.99%           
=======================================
  Files         489      490    +1     
  Lines       28947    28996   +49     
  Branches    13713    13743   +30     
=======================================
+ Hits        14181    14206   +25     
  Misses       4951     4951           
- Partials     9815     9839   +24     

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

/// return true if the assignment should be vetoed
using LocalVeto =
std::function<bool(const MaterialInteraction&,
const std::tuple<const Surface*, Vector3, Vector3>&)>;
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not time critical, so a function is good enough here, I added #include <functional> though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the std::tuple<const Surface*, Vector3, Vector3>. The type looks quite cryptic to me

@asalzburger asalzburger requested a review from andiwand March 27, 2024 08:57
@andiwand
Copy link
Contributor

shall we wait for @Corentin-Allaire to have a look as the expert here?

@Corentin-Allaire
Copy link
Contributor

shall we wait for @Corentin-Allaire to have a look as the expert here?

I have finish all the things that have been draining my time the last few week, so I can have a look this afternoon

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.

Nothing more to add on my side. This is a lot clearer than my implementation and the logic should be exactly the same !

@kodiakhq kodiakhq bot merged commit fc3f250 into acts-project:main Mar 27, 2024
52 checks passed
@asalzburger asalzburger deleted the feat-factorize-material-mapping branch March 27, 2024 20:57
asalzburger added a commit that referenced this pull request Apr 8, 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 #3015 and #3016.


All cases are showcased and tested in a set of UnitTests.
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.0.0 Apr 25, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…faces (MM1) (acts-project#3015)

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, this PR)
3) Mapping those onto dedicated Surface / Volume Material Mappers (MM3)
4) Steer that by a chained algorithm (MM4)

This PR:

The logic how to walk through material interactions and assign those is currently implemented in both the `SurfaceMaterialMapper` and `VolumeMaterialMapper` and consists of many If/else cases that are rather difficult to follow.
This PR (after consultation with @Corentin-Allaire ) introduces a single logic for Surface based material that allows to define global and local vetos (global, e.g. for volume restrictions) and re-assignment policies for pre/post mapping directives.

All cases are showcased and tested in a set of UnitTests.
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
…faces (MM1) (acts-project#3015)

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, this PR)
3) Mapping those onto dedicated Surface / Volume Material Mappers (MM3)
4) Steer that by a chained algorithm (MM4)

This PR:

The logic how to walk through material interactions and assign those is currently implemented in both the `SurfaceMaterialMapper` and `VolumeMaterialMapper` and consists of many If/else cases that are rather difficult to follow.
This PR (after consultation with @Corentin-Allaire ) introduces a single logic for Surface based material that allows to define global and local vetos (global, e.g. for volume restrictions) and re-assignment policies for pre/post mapping directives.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants