-
Notifications
You must be signed in to change notification settings - Fork 301
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
Homogeneous coordinates for field vectors in Mercury iPS #1398
Conversation
Adds functionality to FieldVector that allows for it to be used as a snapshotable parameter, and that adds conversion to and from homogeneous coordinate representations to make it easier to transform FieldVector instances. This functionality is used by the new field-valued parameters of the MercuryiPS instrument. This commit has been made via `git checkout <files> -p` in order to take only changes of interest from a branch that contained more changes. In order to preserve the reference to the original author of this work, the "cherry picked from..." lines are added below. (cherry picked from commit f1bf38b) (cherry picked from commit 3c8a0a1) (cherry picked from commit 251a2f4) (cherry picked from commit b31fa46) (cherry picked from commit 5e23cd6)
Codecov Report
@@ Coverage Diff @@
## master #1398 +/- ##
==========================================
- Coverage 74.13% 74.09% -0.05%
==========================================
Files 85 85
Lines 9822 9883 +61
==========================================
+ Hits 7282 7323 +41
- Misses 2540 2560 +20 |
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.
Looks fine to me. We could add some more validators to the new parameters? @WilliamHPNielsen should probably sign off on this as the original writer of the driver
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.
Apologies for being slow on reviewing this. I generally approve and also just tested it on a real instrument; the example notebook runs with no problems.
As a small style request, I'd like the non-PEP8 compliant spaces before colons to go.
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.
Looks good, thanks, you guys.
This PR adds functionality to
FieldVector
that allows for it to be used as a snapshotable parameter, and that adds conversion to and from homogeneous coordinate representations to make it easier to transformFieldVector
instances. This functionality is used by the new field-valued parameters of theMercuryiPS
instrument.Developed by @cgranade in collaboration with @wpfff.
This functionality is "sort-of-cheery-picked" from #1348. #1348 also contains
async
ramping, but that requires more work, hence this PR. The proposal is to merge this independently fromasync
part.@cgranade Could you have a look at this and tell me if I missed any functionality that enables automated field ramping that you've implemented for pytopo? (without async parts)