-
Notifications
You must be signed in to change notification settings - Fork 344
Add Blocking to DynaCool PPMS During Temperature Ramp #7534
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
base: main
Are you sure you want to change the base?
Add Blocking to DynaCool PPMS During Temperature Ramp #7534
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7534 +/- ##
==========================================
- Coverage 59.89% 59.88% -0.01%
==========================================
Files 352 352
Lines 31806 31812 +6
==========================================
+ Hits 19051 19052 +1
- Misses 12755 12760 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| def _block_on_temperature_ramp(self) -> None: | ||
| """Block all instrument interactions until temperature stabilizes.""" | ||
| while self.temperature_state() != "stable": | ||
| sleep(1) |
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.
let's make this sleep time configurable via an attrbiute on the class?
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.
Added _blocking_t_sleep attribute.
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.
This is better, however it's a private attribute, so as if it's not supposed to be touched by the users, which is not the case, i think it's totally fair that users can adjust that sleep time. So with that logic, i suggest to make it a memory/in-memory parameter (get_cmd=None, set_cmd=None), similar to how it's done with this parameter
| self.ramping_state_check_interval = Parameter( |
| values[self.temp_params.index(param)] = value | ||
|
|
||
| self.write(f"TEMP {values[0]}, {values[1]}, {values[2]}") | ||
| self._block_on_temperature_ramp() |
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.
i'd suggest to leave the current T parameter's implementation as-is in order not to break any existing code in terms of the fact that now setting the temperature blocks; and instead implement a new parameter that includes the "block on ramp". What do you think?
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.
I created a new parameter to address this but also added a default bool to _temp_setter to control blocking functionality without breaking current implementation of the original temperature setting parameters.
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.
I am not a big fan of having two parameters that control the temperature. Which one is the source of truth? I would have made a manual boolean parameter (block_during_temperature_ramp) and then make that one control the behaviour of the existing temperature parameter
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.
My guess is that we're trying to follow the pattern that was established in lakeshore temperature controller driver https://microsoft.github.io/Qcodes/examples/driver_examples/Qcodes%20example%20with%20Lakeshore%20336%20or%20372%20-%20Bluefors%20T%20control.html#Sweeping/blocking-paramameter where there's setpoint parameter that's not blocking, and a blocking_t parameter that does the same as setpoint but also blocks until it's reached.
I'd be in favor of keeping consistency across the drivers (even if it's likely opinionated), so having both setpoint and blocking_t parameters is ok as long as their caches are in sync, i left a separate comment about it.
…revent breaking current implementations.
Testing Results - Bug FoundEnvironment:
Issue:
|
|
@SherwanMicrosoft My last commit resolves this issue. Thank you for the detailed report! |
astafan8
left a comment
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.
a few small comments, but otherwise ready for merge!
|
|
||
| self.blocking_t: Parameter = self.add_parameter( | ||
| "blocking_t", | ||
| label="Block instrument while ramping temp", |
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.
| label="Block instrument while ramping temp", | |
| label="Block instrument while ramping temperature", |
| ) | ||
| """Parameter temperature_setpoint""" | ||
|
|
||
| self.blocking_t: Parameter = self.add_parameter( |
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.
could you mention this parameter in the example notebook https://microsoft.github.io/Qcodes/examples/driver_examples/Qcodes%20example%20with%20DynaCool%20PPMS.html ?
| def _block_on_temperature_ramp(self) -> None: | ||
| """Block all instrument interactions until temperature stabilizes.""" | ||
| while self.temperature_state() != "stable": | ||
| sleep(1) |
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.
This is better, however it's a private attribute, so as if it's not supposed to be touched by the users, which is not the case, i think it's totally fair that users can adjust that sleep time. So with that logic, i suggest to make it a memory/in-memory parameter (get_cmd=None, set_cmd=None), similar to how it's done with this parameter
| self.ramping_state_check_interval = Parameter( |
| values[self.temp_params.index(param)] = value | ||
|
|
||
| self.write(f"TEMP {values[0]}, {values[1]}, {values[2]}") | ||
| self._block_on_temperature_ramp() |
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.
My guess is that we're trying to follow the pattern that was established in lakeshore temperature controller driver https://microsoft.github.io/Qcodes/examples/driver_examples/Qcodes%20example%20with%20Lakeshore%20336%20or%20372%20-%20Bluefors%20T%20control.html#Sweeping/blocking-paramameter where there's setpoint parameter that's not blocking, and a blocking_t parameter that does the same as setpoint but also blocks until it's reached.
I'd be in favor of keeping consistency across the drivers (even if it's likely opinionated), so having both setpoint and blocking_t parameters is ok as long as their caches are in sync, i left a separate comment about it.
| label="Block instrument while ramping temp", | ||
| unit="K", | ||
| vals=vals.Numbers(1.6, 400), | ||
| set_cmd=partial( |
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.
let's make this parameter not gettable (explicitly :) )
| set_cmd=partial( | |
| get_cmd=False, | |
| set_cmd=partial( |
| if block_while_ramping: | ||
| while self.temperature_state() != "stable": | ||
| sleep(self._blocking_t_sleep) | ||
|
|
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.
let's make sure that the caches of the two temperature parameters are in sync when either of them gets set. Note that private method _set_from_raw_value is used since we operate inside this method with raw values (not values, which could have offset/scale), so for correctness reasons we need to use the private method, which is OK to use inside qcodes and inside driver code, just not in user facing measurement code.
| self.setpoint.cache._set_from_raw_value(values[0]) | |
| self.blocking_t.cache._set_from_raw_value(values[0]) |
Summary of Changes