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

refactor(Examples): clean up magnetic field handling #696

Merged
merged 11 commits into from
Feb 11, 2021

Conversation

msmk0
Copy link
Contributor

@msmk0 msmk0 commented Feb 4, 2021

Clean up the magnetic field types and options in the examples:

  • Magnetic field related headers are now consistently located in the ActsExamples/MagneticField include directory and all code is in the ActsExamples namespace. Before, the code still had the old Examples/Plugins/... layout and inconsistent or missing namespacing inherited from the old framework.
  • Move the magnetic field variant type ActsExamples::MagneticField into a separate header.
  • Magnetic field options have a well-documented order of preferrence (constant field overrides field map). If no field options are given, a null field is created, e.g. to support telescope-like detectors without a field. Expected units are clearly communicated as part of the parameter name, similar to what is done in other algorithms.
  • The field map readers are private implementation details and are not publicly available anymore.
  • Detector construction and magnetic field setup are now completely orthogonal: all magnetic fields can be setup for all detectors.

On possible interactions with other open PRs

This is somewhat related to #675 as the magnetic field variant becomes obsolete afterwards. I hope that with the clean up in this PR the move to interface-based magnetic fields will be a bit easier, as the ActsExamples::MagneticField variant type could be replace by something like std::shared_ptr<const Acts::BFieldProvider> without having to rename it. This changeset is part of the ongoing Fatras cleanup.

@msmk0 msmk0 added Component - Examples Affects the Examples module 🚧 WIP Work-in-progress labels Feb 4, 2021
@msmk0 msmk0 added this to the next milestone Feb 4, 2021
@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #696 (6c77383) into master (bd9115d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #696   +/-   ##
=======================================
  Coverage   49.01%   49.01%           
=======================================
  Files         325      325           
  Lines       16574    16574           
  Branches     7744     7744           
=======================================
  Hits         8124     8124           
  Misses       3019     3019           
  Partials     5431     5431           

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 bd9115d...73485e1. Read the comment docs.

@msmk0 msmk0 removed the 🚧 WIP Work-in-progress label Feb 5, 2021
Copy link
Contributor

@XiaocongAi XiaocongAi left a comment

Choose a reason for hiding this comment

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

Hi @msmk0 , it looks all good to me. So I will approve. Meanwhile, I'm wondering if might make sense to move the Examples/Detectors/MagneticField to Examples/MagneticField?

Examples/Detectors/MagneticField/src/FieldMapperRootIo.cpp Outdated Show resolved Hide resolved
Examples/Detectors/MagneticField/src/FieldMapperRootIo.hpp Outdated Show resolved Hide resolved
readRoot = true;
} else if (file.extension() == ".txt") {
ACTS_INFO("Read magnetic field map from text file '" << file << "'");
readRoot = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this line can be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could be omitted. It is here intentionally so both parts of the if/else statement have the same structure and it is easier to understand.

@paulgessinger
Copy link
Member

paulgessinger commented Feb 9, 2021

Can we hold this until I have tested how many merge conflicts I get? I'll try to do that tomorrow.

@msmk0
Copy link
Contributor Author

msmk0 commented Feb 10, 2021

@XiaocongAi Thanks for the review. I'll update the points you raised.

Can we hold this until I have tested how many merge conflicts I get? I'll try to do that tomorrow.

No worries. As previously discussed, I leave the final merge decision/order up to you.

paulgessinger added a commit to paulgessinger/acts that referenced this pull request Feb 10, 2021
@paulgessinger
Copy link
Member

Ok it's not that terrible. I have a rebased branch ready that I believe still works.
So that'll adapt the variant on top of this PR, while still getting the benefits of this PR's other changes.

I'm fine with merging this then.

@msmk0
Copy link
Contributor Author

msmk0 commented Feb 10, 2021

@paulgessinger That is nice to hear.
@asalzburger Do you want to review as well or should we go ahead. @XiaocongAi already approved this PR.

@msmk0
Copy link
Contributor Author

msmk0 commented Feb 10, 2021

Meanwhile, I'm wondering if might make sense to move the Examples/Detectors/MagneticField to Examples/MagneticField?

@XiaocongAi I missed that. I would leave if there for now. The magnetic field is detector-related (to some extend) and there is not really that much code to warrant a completely independent module.

@paulgessinger
Copy link
Member

The header becomes relatively useless anyway with my upcoming changes so I'll probably remove it entirely.

@msmk0 msmk0 requested review from XiaocongAi and removed request for asalzburger February 11, 2021 09:14
@asalzburger asalzburger merged commit 086e721 into acts-project:master Feb 11, 2021
@msmk0 msmk0 deleted the examples-magnetic-field branch February 11, 2021 09:52
paulgessinger added a commit to paulgessinger/acts that referenced this pull request Feb 15, 2021
paulgessinger added a commit to paulgessinger/acts that referenced this pull request Feb 24, 2021
@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
Labels
Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants