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: Add covfie magnetic field plugin #3479

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

stephenswat
Copy link
Member

@stephenswat stephenswat commented Aug 5, 2024

This commit adds a new Acts plugin that adds support for magnetic fields implemented using the covfie library.

This commit is based on #3117.

Closes #3117.

Depends on #3478.

Virtually all credit goes to @fredevb.

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Component - Documentation Affects the documentation labels Aug 5, 2024
@paulgessinger
Copy link
Member

Can you update the branch? I'll look at this today

@stephenswat stephenswat force-pushed the feat/new_covfie_plugin branch from b0769f0 to 4cd2105 Compare August 22, 2024 13:04
@github-actions github-actions bot removed the Component - Documentation Affects the documentation label Aug 22, 2024
@stephenswat
Copy link
Member Author

This is now ready for review.

Copy link

github-actions bot commented Aug 22, 2024

📊: Physics performance monitoring for 37d0332

Full contents

physmon summary

@paulgessinger paulgessinger self-requested a review August 24, 2024 08:20
Examples/Python/src/Covfie.cpp Outdated Show resolved Hide resolved
Examples/Python/src/Covfie.cpp Outdated Show resolved Hide resolved
Examples/Python/src/CovfieStub.cpp Outdated Show resolved Hide resolved
Examples/Python/src/MagneticField.cpp Outdated Show resolved Hide resolved
Plugins/Covfie/src/FieldConversion.cpp Outdated Show resolved Hide resolved
Plugins/Covfie/src/FieldConversion.cpp Show resolved Hide resolved
stephenswat added a commit to stephenswat/acts that referenced this pull request Aug 26, 2024
As acts-project#3479 reveals, we don't currently have any clean, cache-aware ways of
accessing B-fields in Python code. In order to avoid hacks, this commit
adds the necessary bindings to allow us to cleanly access B-fields with
cache objects.
@paulgessinger paulgessinger added this to the next milestone Aug 26, 2024
@stephenswat stephenswat force-pushed the feat/new_covfie_plugin branch from 4cd2105 to 8cea693 Compare August 27, 2024 08:53
@stephenswat stephenswat marked this pull request as draft August 27, 2024 08:53
kodiakhq bot pushed a commit that referenced this pull request Aug 27, 2024
As #3479 reveals, we don't currently have any clean, cache-aware ways of accessing B-fields in Python code. In order to avoid hacks, this commit adds the necessary bindings to allow us to cleanly access B-fields with cache objects.
@stephenswat stephenswat force-pushed the feat/new_covfie_plugin branch from 8cea693 to 027a574 Compare August 27, 2024 11:14
@stephenswat stephenswat marked this pull request as ready for review August 27, 2024 11:14
@stephenswat
Copy link
Member Author

@paulgessinger I have incorporated your feedback and rebased.

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

I'm not completely sure I understand how you get around the out-of-bounds field access with the interpolated fields now.

Plugins/Covfie/src/FieldConversion.cpp Outdated Show resolved Hide resolved
Plugins/Covfie/src/FieldConversion.cpp Outdated Show resolved Hide resolved
Plugins/Covfie/src/FieldConversion.cpp Show resolved Hide resolved
@stephenswat
Copy link
Member Author

stephenswat commented Aug 27, 2024

I'm not completely sure I understand how you get around the out-of-bounds field access with the interpolated fields now.

The core problem is that ACTS won't allow me to access the points at the edge of a B-field. In a one-dimensional example, if I define a data series as $[2, 3, 2, 1]$, I can ask ACTS for the position at $x = 0$ and get $f(0) = 2$; I can also ask for e.g. $f(1.5) = 2.5$. But I cannot ask for $f(3)$ (which would be $1$) because of this line: https://github.com/acts-project/acts/blob/main/Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp#L256. Because $3$ is the boundary and the convention is that the range is half-open, this returns an error. My understanding is that this is by design.

The way I have solved (c.q. hacked) this is by asking for $f(0)$, $f(1)$, $f(2)$ as normal and then - when I get to $x = 3$, asking for $f(x')$ where $x'$ is the closest representable number that is smaller than $3$; so I end up asking for $f(2.999999999)$ or something like that.

Not super clean (actually quite disgusting) but it works. I investigated accessing the underlying grid directly, but this is not generally supported by ACTS B-field classes, unfortunately.

@stephenswat stephenswat force-pushed the feat/new_covfie_plugin branch from 027a574 to 9b1974d Compare August 27, 2024 14:30
@stephenswat
Copy link
Member Author

I've resolved the comment issues; I've left them in even if they are not necessary because, well, why not. 😄

@paulgessinger
Copy link
Member

Ok, looks good. There's a few CI failures that need fixing, otherwise it's good to go I think.

@stephenswat stephenswat force-pushed the feat/new_covfie_plugin branch from 9b1974d to b0346cb Compare August 27, 2024 22:04
@paulgessinger
Copy link
Member

There's just one remaining clang tidy item about inconsistent argument names between declaration and definition.

@stephenswat stephenswat force-pushed the feat/new_covfie_plugin branch from b0346cb to d0acf60 Compare August 28, 2024 13:31
This commit adds a new Acts plugin that adds support for magnetic fields
implemented using the covfie library.

Co-authored-by: Stephen Nicholas Swatman <[email protected]>
Copy link

@kodiakhq kodiakhq bot merged commit 809b378 into acts-project:main Aug 29, 2024
42 checks passed
@andiwand andiwand modified the milestones: next, v36.3.0 Sep 2, 2024
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 Component - Plugins Affects one or more Plugins Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants