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

driver / add block_during_ramp parameter to AMI430_3D #1228

Merged
merged 3 commits into from
Aug 14, 2018
Merged

driver / add block_during_ramp parameter to AMI430_3D #1228

merged 3 commits into from
Aug 14, 2018

Conversation

emartinez
Copy link
Contributor

@emartinez emartinez commented Aug 13, 2018

@WilliamHPNielsen

The AMI430_3D magnet does not currently support non-blocking field ramps. A linear field ramp between two safe points is always safe if the ramp rates are safe, so this should be possible.

The new block_during_ramp parameter (default True), if set to False, will call the set_field methods of the children with the argument block=False, so that they get ramped in a non-blocking way.

@WilliamHPNielsen WilliamHPNielsen changed the title Added block_during_ramp parameter to AMI430_3D parameter. driver / add block_during_ramp parameter to AMI430_3D Aug 13, 2018
@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #1228 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1228      +/-   ##
=========================================
+ Coverage   80.48%   80.5%   +0.02%     
=========================================
  Files          49      49              
  Lines        6804    6807       +3     
=========================================
+ Hits         5476    5480       +4     
+ Misses       1328    1327       -1

@sohailc
Copy link
Member

sohailc commented Aug 13, 2018

@emartinez wrote:

A linear field ramp between two safe points is always safe if the ramp rates are safe, so this should be possible.

Not True! This assumes that the safe region is convex. We know that this is not always the case.

@WilliamHPNielsen
Copy link
Contributor

WilliamHPNielsen commented Aug 13, 2018

@sohailc how was it again..? The sequential ramping keeps you safe only if the safe region is star-shaped around the origin, right? This is more general than convex, but can also in principle be false. But then I guess all bets are off. [EDIT: no, wait, star-shaped is too weak... is it convex also for sequential ramping?]

In any case, before we merge, let me extend the tests to cover this new parameter.

@emartinez
Copy link
Contributor Author

@sohailc wrote:

Not True! This assumes that the safe region is convex. We know that this is not always the case.

All the BlueFors AMI magnet specifications in Copenhagen state:

This magnet can be ramped at any combination of X/Y/Z simultaneous fields starting from zero to a maximum of +/-60, +/-10, +/-10 kG, respectively. The ramp rates of the individual X/Y/Z coils must be adjusted so that the ramp time from zero field to their desired target fields is the same for al coils.

I.e. the safe region is a right rectangular prism, which is convex. We know from the magnet acceptance test that the corners of this prism have been explored and are safe.

If this is not always the case, then I would suggest to subclass the driver so that users of less exotic magnets (at least 5 fridges here) can benefit from reasonable safety assumptions.

@WilliamHPNielsen WilliamHPNielsen merged commit 000c451 into microsoft:master Aug 14, 2018
@WilliamHPNielsen
Copy link
Contributor

@sohailc @emartinez I guess what I am trying to ask is: are there realistic use cases where a linear ramp is not safe but a first-down-then-up approach is? I can think of a few examples, but they are quite contrived. Unless the first-down-then-up actually goes all the way to the origin and then out to the target field value, I guess there generally isn't.

giulioungaretti pushed a commit that referenced this pull request Aug 14, 2018
Merge: b68979e b27f36c
Author: William H.P. Nielsen <whpn@mailbox.org>

    Merge pull request #1228 from emartinez/driver/AMI340_non_blocking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants