[WIP] Driver/lakeshore336 and lakeshore 372 #972
Conversation
| r: "{}" | ||
| setter: | ||
| q: "INNAME A,\"{}\"" | ||
| r: OK |
There was a problem hiding this comment.
This "OK" response is now obsolete (#968).
There was a problem hiding this comment.
I dont know if there was an actual test file using this file that I missed in the rebase
| from qcodes.utils.validators import Enum, Strings | ||
| from qcodes.utils.validators import Enum as QCEnum, Strings | ||
|
|
||
| class ChannelDescriptor(Enum): |
There was a problem hiding this comment.
I may be missing something important, but this seems like an unnecessary complication to add. The cons are: reduced code readability (renaming Enum to QCEnum, having to look up ChannelDescriptor).
What are the pros?
In any case, the naming conflict is easily resolved by either just importing enum or importing qcodes.utils.validators as vals (or both). QCEnum is an abonimation.
There was a problem hiding this comment.
Agreed, QCEnum is a really bad name - the state of the code is really WIP.
But how is it a complication? I agree that it could also just be a dictionary. But does this make it simpler?
There was a problem hiding this comment.
Just for the record: we agreed that changing the import names and keeping using the enum from the standard library is the way to go.
Codecov Report
@@ Coverage Diff @@
## master #972 +/- ##
==========================================
+ Coverage 70.92% 70.99% +0.06%
==========================================
Files 74 74
Lines 8352 8375 +23
==========================================
+ Hits 5924 5946 +22
- Misses 2428 2429 +1 |
|
There are some missing |
|
The driver is in a usable state now. It will need some more refactoring. The two models are not that similar after all. There are a few things that have to be moved out of the base class into the 336 like the setpoints. |
| set_cmd=f'RANGE {heater_index}, {{}}', | ||
| get_cmd=f'RANGE? {heater_index}') | ||
|
|
||
| self.add_parameter('setpoint', get_parser=lambda x: Range(int(x)), |
There was a problem hiding this comment.
This has get parser twice
|
@Dominik-Vogel This will need a bit of work to get the tests passing before we can review it |
|
It is actually not as bad as it seems... I think it is only the repeated keyword (I simply forgot to delete the first get_cmd in the last commit) and the unused imports. Or do you see other errors? |
Dominik-Vogel
changed the title
|
Currently both the tests and mypy fails |
|
This is not very surprising, my last commit is also labeled WIP and is in the middle of refactoring. |
|
Yes but the comment before that implied that it was ready for review, hence my message |
Dominik-Vogel
changed the title
|
@QCoDeS/core ready for review |
There was a problem hiding this comment.
As discussed with @Dominik-Vogel, i will test it in the lab with the actual fridge. Once i'm done with the testing, I'll remove the "request changes" note.
| self.I.vals = vals.Numbers(0.1, 1000) | ||
| self.D.vals = vals.Numbers(0, 200) | ||
|
|
||
| self.range_limits.vals = validators.Sequence( |
There was a problem hiding this comment.
Error: validators is unknown. This should be vals.Sequence(...
| log = logging.getLogger(__name__) | ||
|
|
||
| class Output_336(BaseOutput): | ||
|
|
There was a problem hiding this comment.
Can we add a docstring describing the usefulness of this class? This is not clear to me.
| 'off': 0, | ||
| 'low': 1, | ||
| 'medium': 2, | ||
| 'heigh': 3} |
There was a problem hiding this comment.
typo in "heigh". Should be "high"
| self.parameters = OrderedDict((p.name, p) for p in parameters) | ||
| self.instrument = parameters[0].root_instrument | ||
| for p in parameters: | ||
| p.group = self |
There was a problem hiding this comment.
There should be a public method called set_group on the GroupParameter class to do this. Do not assign attributes directly like this.
There was a problem hiding this comment.
In fact, if you make a parameter group like so:
self._group_param = Group(
set_cmd="bla {{a}} {{b}}"
)
self.add_parameter(
"a",
group=self._group_param,
parameter_class=GroupParameter
)
Then line 35 will not be necessary and we will not need to set the group attribute because we pass it to the constructor. I think this is more clear.
There was a problem hiding this comment.
There is, as far as I can see, a mutual dependence of the parameter and the group: The parameter has to know the group, and the group has to know the parameter.
This is because we want to call set/get on the parameter and it and needs the values of the other parameters to accomplish that.
So this double link is created in an init method. This can equally happen in the parameter or in the group. In your example the parameter is aware of the group but the group is not aware of the parameter.
I chose to put this linking into the group because it makes the code a little more compact and readable for my taste... But I can be convinced otherwise if you include the previous aspect in your argument.
concerning the attribute I wrote you a PM.
There was a problem hiding this comment.
I think you are right about this. I think if you clean this code up a little (e.g. add docstrings, proper type hinting) it can be approved and merged. But I am still unsure if we should have a setter for the group or not. I read the link you send me on PM, but I still think having a setter will make things more clear to fellow programmers.
| self.add_parameter('powerup_enable', | ||
| val_mapping={True: 1, False: 0}, | ||
| parameter_class=GroupParameter) | ||
| self.output_group = Group([self.mode, self.input_channel, |
There was a problem hiding this comment.
I quite like the way the group parameter works. I think we should make this available generally to other instruments as well.
There was a problem hiding this comment.
Thank you @sohailc . I was also thinking that and wanted to use the Lakeshore as a testground first. I think it could maybe make a module together with the alazar parameters with the context manager for syncing that @jenshnielsen introduced. The way of defining a group is similar and there could be a keyword to determine whether parameters get synced on demand (like in alazar) or automatically (like here)
| for name, p in self.parameters.items()} | ||
| calling_dict[set_parameter.name] = value | ||
| command_str = self.set_cmd.format(**calling_dict) | ||
| set_parameter.root_instrument.write(command_str) |
There was a problem hiding this comment.
why not self.instrument.write(command_str)?
There was a problem hiding this comment.
to keep things simple, i will use instrument. also, i will add to the docstring that it is assumed that all parameters in the group belong to the same instrument
| calling_dict = {name: p.raw_value | ||
| for name, p in self.parameters.items()} | ||
| calling_dict[set_parameter.name] = value | ||
| command_str = self.set_cmd.format(**calling_dict) |
There was a problem hiding this comment.
this means that the Group only supports string commands, right? Shall we generalize it to callables as well?
There was a problem hiding this comment.
maybe later, but not in this PR
| return dict(zip(keys, values)) | ||
| return parser | ||
|
|
||
| def set(self, set_parameter, value): |
There was a problem hiding this comment.
is get method missing? does it mean that instead of using param_group.get() which presumable returns a param-values dict, do i need to call get of every parameter in the group? in this case, will the "update" from the instrument happen for every parameter?
There was a problem hiding this comment.
param_group.get() does not seem to be a use case. and indeed, update is called every time one of the parameter's get method is called.
|
Testing of this driver for some simply/quick measurements has begun on QT3. As you can see, we already found one (rather crucial) bug. If the driver suddenly becomes important, I might take over and fix the issues pointed out by @astafan8 above. |
| from contextlib import suppress | ||
| import time | ||
|
|
||
| # from qcodes.instrument_drivers.Lakeshore.Model_336 import Model_336 |
| self.wait_equilibration_time(0.5) | ||
|
|
||
|
|
||
| self.add_parameter('blocking_T', |
There was a problem hiding this comment.
shouldn't this be a method? (by astafan8)
There was a problem hiding this comment.
old Loop needs a parameter, so we end up with a parameter on top of a method instead of a method-only.
|
Today I've executed the driver on an actual Lakeshore Model 372 instrument. Outcome:
|
| start_time_in_tolerance_zone = None | ||
|
|
||
| log.debug(f'waiting to reach setpoint: temp at ' | ||
| f'{t_reading}, delta:{delta}') |
There was a problem hiding this comment.
this line throws exception about unknown variable delta in case t_reading is None/False/etc. Also, why would we have if t_reading: condition in the first place?
There was a problem hiding this comment.
fixed, if t_reading: is gone
If setpoint is in a different range than current range, then waiting might happen forever because the setpoint cannot be reached within the current range.
# Conflicts: # qcodes/instrument/group_parameter.py
|
Dear @QCoDeS/core , I would like to wrap up my work on this driver for now. See the description for the list of items that have been done (and more). Please, review and leave your comments. I propose that after testing the two drivers on the real devices, we merge this PR, and create an issue for the bullet items which are not covered in this PR. This is bad, I know, but this will allow our users to use the new driver (at the moment, they are using it via merging this branch into their other branches). |
|
@QCoDeS/core The 372 driver is in active use now at two stations in Delft, everything works as expected and the users are passing their gratitude to those who worked on the driver :) So shall we merge it "as is" (see my previous message for details)? |
|
@astafan8, can we make the CI pass first? |
|
Sorry guys I didn't have time to get back to this one... Let me see if I can get the ci through and then merge it |
|
Now all ci passes. It would be great to get a confirming review ;-) so that I can merge |
|
You don't need that for merging. |
A driver for Lakeshore Temperature Controllers 336 and 372. From QDev probe station by @Dominik-Vogel.
Includes a great new
GroupParamter+Groupclasses for commands that set/get more than one parameter at once.https://www.lakeshore.com/Documents/372_Manual.pdf
https://www.lakeshore.com/Documents/336_Manual.pdf
ToDo:
_has_pida kwargblocking_tpartwait_until_set_point_reachedsimplify and remove debugwait_until_set_point_reached- if t is lower than sensor range, it keeps on waiting - add a noteWontDoInThisPR:
valsforexcitation_range_numberbased onexcitation_mode(and then remove the big comment in the code)