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!: B field access returns Result #825

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Jun 2, 2021

Previously, we didn't have a mechanism to communicate that a field query failed. This is problematic, because it's not clear what to do when for instance our interpolated field goes out of bounds (see #784). This PR makes MagneticFieldProvider require the getField and getFieldGradient methods to return Result<Vector3> instead of the bare vector. This changes the public interface slightly, and all clients need to be adjusted.

Additionally, the InterpolatedBFieldMap now checks each time a field cell is created or if the field is queried directly, whether the query position is out of bounds of the map, and will return a new MagneticFieldError::OutOfBounds if that happens.

BREAKING CHANGE:

  • The signature of field query methods in MagneticFieldProvider changes from
    virtual Vector3 getField(const Vector3& position, Cache& cache) const = 0;
    virtual Vector3 getFieldGradient(const Vector3& position, ActsMatrix<3, 3>& derivative, Cache& cache) const = 0;
    to
    virtual Result<Vector3> getField(const Vector3& position, Cache& cache) const = 0;
    virtual Result<Vector3> getFieldGradient(const Vector3& position, ActsMatrix<3, 3>& derivative, Cache& cache) const = 0;
  • InterpolatedBFieldMap::getMin and InterpolatedBFieldMap::getMax now return the extent of the valid interpolation domain, rather than the raw grid extent.

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #825 (6ed2a7f) into main (bdd59e0) will decrease coverage by 0.13%.
The diff coverage is 26.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #825      +/-   ##
==========================================
- Coverage   48.68%   48.54%   -0.14%     
==========================================
  Files         327      328       +1     
  Lines       16893    16958      +65     
  Branches     7945     7999      +54     
==========================================
+ Hits         8224     8233       +9     
- Misses       3066     3082      +16     
- Partials     5603     5643      +40     
Impacted Files Coverage Δ
Core/include/Acts/MagneticField/SolenoidBField.hpp 100.00% <ø> (ø)
Core/include/Acts/Propagator/Propagator.ipp 37.17% <0.00%> (ø)
.../include/Acts/Propagator/detail/LoopProtection.hpp 42.10% <0.00%> (-17.90%) ⬇️
.../include/Acts/Vertexing/HelicalTrackLinearizer.ipp 27.83% <0.00%> (-1.96%) ⬇️
...re/include/Acts/Vertexing/ImpactPointEstimator.ipp 36.25% <0.00%> (-0.65%) ⬇️
Core/src/MagneticField/MagneticFieldError.cpp 20.00% <20.00%> (ø)
Core/include/Acts/Propagator/AtlasStepper.hpp 68.55% <23.52%> (-0.82%) ⬇️
Core/include/Acts/Propagator/EigenStepper.ipp 49.61% <24.00%> (-3.49%) ⬇️
Core/include/Acts/MagneticField/ConstantBField.hpp 70.00% <25.00%> (-10.00%) ⬇️
Core/include/Acts/MagneticField/NullBField.hpp 50.00% <25.00%> (-12.50%) ⬇️
... and 7 more

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 bdd59e0...6ed2a7f. Read the comment docs.

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Jun 3, 2021
@paulgessinger
Copy link
Member Author

paulgessinger commented Jun 3, 2021

Turns out the interpolated b field test was actually wrong.
It was setting up the map like

detail::EquidistantAxis r(0.0, 4.0, 4u);
detail::EquidistantAxis z(-5, 5, 5u);

and then asserted that the map went from r=0-4 and z=-5,5, when in fact it went from r=0-3 and z=-5,3 because the upper edge of the outer bins doesn't have a value anymore, so the interpolation domain ends one bin lower. This PR now fixes this test.

Also, InterpolatedBFieldMap::isInside, getMin, getMax and InterpolatedBFieldMapper::isInside reported this wrong interpolation domain based on the raw extent of the underlying grid. Those should all be fixed now.

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Jun 3, 2021
@paulgessinger
Copy link
Member Author

Ok, I think now it should be ready, sorry for the flip flop @HadrienG2

Copy link
Contributor

@HadrienG2 HadrienG2 left a comment

Choose a reason for hiding this comment

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

SGTM

@paulgessinger paulgessinger merged commit b6371e2 into acts-project:main Jun 4, 2021
@paulgessinger paulgessinger deleted the bfield-access-can-fail branch June 4, 2021 07:19
@paulgessinger paulgessinger modified the milestones: next, v9.0.0 Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants