Add simultaneous blocking ramp parameters MercuryIPS #1467
Conversation
… magnet to be set and ramped with a single call
Codecov Report
@@ Coverage Diff @@
## master #1467 +/- ##
=======================================
Coverage 73.86% 73.86%
=======================================
Files 92 92
Lines 10411 10411
=======================================
Hits 7690 7690
Misses 2721 2721 |
| for slave in self.submodules.values(): | ||
| slave.ramp_to_target() | ||
|
|
||
| for slave in np.array(list(self.submodules.values())): |
There was a problem hiding this comment.
why not for slave in self.submodules.values():?
|
|
||
| # then the actual ramp | ||
| {'simul': self._ramp_simultaneously, | ||
| 'safe': self._ramp_safely}[mode]() | ||
| 'safe': self._ramp_safely, | ||
| 'simul_block':self._ramp_simultaneously_blocking}[mode]() |
There was a problem hiding this comment.
there is some weird excessive indentation in the whole commit, could this be fixed for readability?
| slave.ramp_to_target() | ||
|
|
||
| for slave in np.array(list(self.submodules.values())): | ||
| # wait for the ramp to finish, we don't car about the order |
There was a problem hiding this comment.
| # wait for the ramp to finish, we don't car about the order | |
| # wait for the ramp to finish, we don't care about the order |
| out of your safe region. Use with care. This function is BLOCKING. | ||
| """ | ||
| for slave in self.submodules.values(): | ||
| slave.ramp_to_target() |
There was a problem hiding this comment.
these two lines can be safely substituted with self._ramp_simultaneously(), right? This reduces code duplication, makes the code more readable.
| self.ramp(mode='safe') | ||
| self.theta_measured() | ||
|
|
||
| def _theta_saferamp_get(self): |
There was a problem hiding this comment.
what are these get* methods needed for if they just refer to the parameters which actually represent the values (like theta_measured)? I'd suggest to use set-only parameters for the ramping, and if values need to be read, then appropriate corresponding parameters are used. The reason for this is to remove redundancy if it brings extra maintainability and perhaps confusion.
| set_cmd=self._r_saferamp_set, | ||
| get_cmd=self._r_saferamp_get) | ||
|
|
||
| self.add_parameter('r_simulramp', |
There was a problem hiding this comment.
shall there be also phi_simulramp and theta_simulramp? Also, shall there be corresponding parameters for x/y/z/rho?
| """ | ||
| self.theta_target(target) | ||
| self.ramp(mode='safe') | ||
| self.theta_measured() |
There was a problem hiding this comment.
what is this call needed for? in order to update the value of the parameter, i guess? how is it used by Loop?
There was a problem hiding this comment.
Thanks for the comments, mostly it was written by copy/pasting. I'll make some improvements and resubmit it
WilliamHPNielsen
changed the title
|
I have added a couple of commits, that I think address the comments. For the blocking ramp parameters (X_saferamp etc.), these were primarily useful for the legacy dataset as convenience functions. They are less useful for the new dataset where it's easier to reason inside a loop. However, I think a universal update call is important and it should be called at the end of the blocking ramps. Otherwise the coordinates get stuck in an inconsistent state, that gets written to metadata etc., updating this shouldn't be left to the user. Perhaps later the code could be refactored in terms of properties, but this seems a reasonable workaround for now |
|
Sorry, I just fixed a couple more bugs. |
... instead of multiline lambdas. This is more readable/pythonic and removes mypy errors.
|
@ectof I made a commit because type checker was showing errors - using private method instead of lambdas. Could you quickly test this change on the real instrument? |
|
@astafan8 I just checked on the instrument and it works as expected. Thanks! |
Added additional parameters to oxford\MercuryIPS_VISA.py allowing the magnet to be set and ramped with a single call. This is useful when running in a loop.
@WilliamHPNielsen