-
Notifications
You must be signed in to change notification settings - Fork 168
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
refactor(Examples): clean up magnetic field handling #696
Conversation
Codecov Report
@@ 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.
|
eb85f08
to
cead8ab
Compare
There was a problem hiding this 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
?
readRoot = true; | ||
} else if (file.extension() == ".txt") { | ||
ACTS_INFO("Read magnetic field map from text file '" << file << "'"); | ||
readRoot = false; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Can we hold this until I have tested how many merge conflicts I get? I'll try to do that tomorrow. |
@XiaocongAi Thanks for the review. I'll update the points you raised.
No worries. As previously discussed, I leave the final merge decision/order up to you. |
Co-authored-by: Xiaocong Ai <xiaocong.ai@cern.ch>
Ok it's not that terrible. I have a rebased branch ready that I believe still works. I'm fine with merging this then. |
@paulgessinger That is nice to hear. |
@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. |
The header becomes relatively useless anyway with my upcoming changes so I'll probably remove it entirely. |
Clean up the magnetic field types and options in the examples:
ActsExamples/MagneticField
include directory and all code is in theActsExamples
namespace. Before, the code still had the oldExamples/Plugins/...
layout and inconsistent or missing namespacing inherited from the old framework.ActsExamples::MagneticField
into a separate header.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 likestd::shared_ptr<const Acts::BFieldProvider>
without having to rename it. This changeset is part of the ongoing Fatras cleanup.