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.

Sign up for GitHub

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

Jump to bottom

feat(Fatras): Photon conversion #597

Merged
merged 23 commits into from
Feb 8, 2021
Merged

feat(Fatras): Photon conversion #597

merged 23 commits into from
Feb 8, 2021

Conversation

FabianKlimpel
Copy link
Contributor

@FabianKlimpel FabianKlimpel commented Nov 23, 2020 •
edited by msmk0
Loading

This PR adds the photon conversion to ActsFatras. This class is a decomposition of the corresponding Geant4 class. Due to #588 this PR remains WIP until the required interface is available.

Closes #189.

@FabianKlimpel FabianKlimpel added this to the next milestone Nov 23, 2020
@FabianKlimpel FabianKlimpel added 🚧 WIP Work-in-progress Component - Fatras Affects the Fatras module Feature Development to integrate a new feature labels Nov 23, 2020
@codecov
Copy link

codecov bot commented Nov 23, 2020 •
edited
Loading

Codecov Report

Merging #597 (d62db1c) into master (040a098) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #597   +/-   ##
=======================================
  Coverage   49.00%   49.00%           
=======================================
  Files         331      331           
  Lines       16549    16549           
  Branches     7723     7723           
=======================================
  Hits         8110     8110           
  Misses       3009     3009           
  Partials     5430     5430

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 040a098...971dbee. Read the comment docs.

@stale
Copy link

stale bot commented Jan 14, 2021

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale label Jan 14, 2021
@stale stale bot removed the Stale label Feb 2, 2021
@FabianKlimpel FabianKlimpel removed the 🚧 WIP Work-in-progress label Feb 2, 2021
@msmk0 msmk0 changed the title feat: Photon conversion feat(Fatras): Photon conversion Feb 2, 2021
msmk0
msmk0 suggested changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

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

@FabianKlimpel Thanks for updating this to the new interfaces. The general structure looks good. I did not check the numerics but I assume that they are just copied from existing code. I have some requests on naming and some implementation details below.

I would also ask you to add some unit tests for this physics module. Either as a regression test against a well-known setup or as consistency tests for one or two corner cases, e.g. energy-momentum relation is conserved, direction is reasonable, particle ids are correct. Have a look at how this is done for the existing energy loss and scattering modules.

I saw a few comments that some of the code originates from Geant4. What license was the code initially under?

Core/include/Acts/Propagator/EigenStepper.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/StraightLineStepper.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/EventData/ProcessType.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
@msmk0 msmk0 mentioned this pull request Feb 3, 2021
Merged
@FabianKlimpel FabianKlimpel force-pushed the PhotonConversion branch 3 times, most recently from b38ed63 to 146a4be Compare February 4, 2021 14:50
msmk0
msmk0 suggested changes Feb 5, 2021
View reviewed changes
Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

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

@FabianKlimpel Thanks for all the fixes and for adding the unit tests. I have some minor comments and I would recommend to split the single unit test case into multiple ones. After that, this can go in.

Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Show resolved Hide resolved
Fatras/include/ActsFatras/Physics/PhotonConversion/PhotonConversion.hpp Outdated Show resolved Hide resolved
Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Show resolved Hide resolved
Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Outdated Show resolved Hide resolved
msmk0
msmk0 approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

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

@FabianKlimpel Thanks for all the changes. This looks great and can go in. I have found two points in the tests where the documentation was wrong, that could be fixed.

Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Show resolved Hide resolved
Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Show resolved Hide resolved
@asalzburger
Copy link
Contributor

@FabianKlimpel can you resolve the conflicts, and I will merge.

msmk0
msmk0 suggested changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

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

@FabianKlimpel The test doc is wrong again.

Tests/UnitTests/Fatras/Physics/PhotonConversionTests.cpp Show resolved Hide resolved
@msmk0 msmk0 merged commit 5c6bbbb into acts-project:master Feb 8, 2021
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Feb 9, 2021
@FabianKlimpel @paulgessinger
feat(Fatras): Photon conversion (acts-project#597)
edc7efd 
Add photon conversion to Fatras.
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Feb 10, 2021
@FabianKlimpel @paulgessinger
feat(Fatras): Photon conversion (acts-project#597)
20a4c6a 
Add photon conversion to Fatras.
@msmk0 msmk0 mentioned this pull request Feb 21, 2021
Merged
paulgessinger pushed a commit that referenced this pull request Mar 1, 2021
@msmk0
feat(Examples): Add photon conversion to Fatras simulation (#722)
213d97d 
Add the photon conversion process to the Fatras algorithm in the examples. The photon conversion implementation itself was previously added to Fatras in #597. This adds the command line option and necessary implementation details in the examples algorithm.

This also switches constants in the photon conversion implementation to `static const` to make the process compatible with the physics list. Removing the constant member variables re-enables all default implicit constructors and assignment operators.

Depends on #726 and can only be merged afterwards.
@paulgessinger paulgessinger modified the milestones: next, v6.0.0 Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msmk0 msmk0 msmk0 approved these changes

Assignees
No one assigned
Labels
Component - Fatras Affects the Fatras module Feature Development to integrate a new feature
Projects
None yet
Milestone
v6.0.0
Development

Successfully merging this pull request may close these issues.

Add EM conversions to the Fatras simulation
4 participants
@FabianKlimpel @asalzburger @msmk0 @paulgessinger