SRS 830 LockIn Driver#139
Conversation
Add a validator for boolean type values such as on/off, true/false etc. TODO: Follow this up with a getter/setter that can handle these values as defined here
Initial commit for the SRS 830 Lockin Amplifier. Still a couple of TODO's before it is feature complete however there is enough here to start taking data off instruments.
|
@spauka cool! The driver looks very good so far. We do have a My own preference would be to always use |
|
|
||
| def set_tc(self, val): | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
minor style point - can you set your editor to always strip trailing whitespace on save? It just makes awkward diffs in the future when someone else's unrelated commit does it.
|
@alexcjohnson , I agree on having a convention for Bools. However the problem I see is that instruments usually have a status parameter, which can then be either So what convention do you propose? |
|
The main point of my comment is I don't think we should be allowing multiple values that mean the same thing when setting a parameter. If we let every user choose whether to set the parameter You're right, The possibility of values proliferating later on is a good point - for some parameters it would probably be hard to imagine the hardware changing to accept more than two options, so you can safely name them to be clearly boolean and move on. And if a parameter like THAT suddenly accepts a third value you should change its name. So for the old AWG, you could make a As a general convention for NON-boolean enums, I would propose the shortest clear lower-case word. I know @MerlinSmiles will complain about the keystrokes, he wants to type |
|
Agreed regarding bool for filter states. I seems like now would be a good time to have a discussion about standardizing some of these naming/typing conventions. I suggest |
I oppose changing variable names for instrument driers for the sake of Booleans. I think the name should be as close as possible to the one that is used in the manual of the specific instrument.
In fact all the MW-sources I have written have the parameter
I would propose allowing any capitalization and using .lower on the input to convert it to all lowercase. So to summarize what I propose
|
Sounds like a good rule of thumb. That means most parameters like this will end up with string values rather than booleans, which also has the advantage of being more future-proof in case more options are added.
Yes, I had seen you do that - I agree. That should satisfy @MerlinSmiles 's fingers while maintaining
This would be more than just a validator change - it would need to change the value you, the user, supplied, so that the stored (get_latest, or snapshot) value is consistently lower case too. I'm not a big fan of this, it adds complexity and could still lead to confusing bugs like this infinite loop: while inst.status() != 'On':
inst.status('On')What's wrong with just standardizing on lowercase? People will learn quickly: when the validator rejects their capitalized word, it tells them what the allowed values are. |
Create a parser that checks for integer matches in the get_parser when a val_mapping is given for an instrument.
| return self._get_mapping[val] | ||
| except (ValueError, KeyError): | ||
| raise KeyError("Unmapped value from instrument: {:r}".format(val)) | ||
|
|
There was a problem hiding this comment.
@spauka this looks reasonable, but can you give me an example or two of how you intend to use this? ie what are the specific commands you pass to the instrument on setting, responses on getting, and the corresponding val_mapping?
There was a problem hiding this comment.
This is used in a couple of places where the lockin uses integers to set some flag.
Used for mappings such as:
ILIN = {'Off': 0, 'Line': 1, '2xLine': 2, 'Both': 3}
RMOD = {'High_Reserve': 0, 'Normal': 1, 'Low_Noise': 2}It would be possible to make the dictionaries map to strings instead, however it seems more logical to allow mapping to integers given that that is how they are described in the docs.
There was a problem hiding this comment.
Good - I think this is nice, integer codes come up all the time. I've added a test of it qdev-dk-archive@9f6cca9
Another way to do this would be to manipulate _get_mapping - since the input to this mapping (a raw return from the instrument) is always going to be a string, we could just force that from the outset:
self._get_mapping = {str(v): k for k, v in val_mapping.items()}
This would be a bit faster, since get doesn't have to go through the try/except, but it would behave differently in some cases: if the instrument for some reason padded its response with whitespace for example, your version would still work but not mine. Commands like this are unlikely to be time bottlenecks, I would imagine (they'll rarely be in the inner loop of an acquisition... so perhaps the whitespace behavior is enough reason to keep it as is.
|
I merged the functionality of spauka's and my SR830 driver. All parameters like sensitivity and time constant are now set using numbers, and have the appropriate units for measuring either voltages or current. |
|
@spauka I haven't looked in detail at this yet, but I encouraged @Rubenknex to bring his work into this PR. Can you take a look at it now and see if you're happy with it all? |
|
I would express a personal preference for capitalizing parameters (ground->Ground), particularly in the case of initialisms (ac->AC, dc->DC). was looking over other drivers but there does not seem to much precedence here. I would also express a preference for calling the company SRS, if for no other reason that the class name for the instrument becomes pretty long and inconvenient to type. I think we had a discussion at some point about instrument names, don't recall if this was raised? Other than that, once missing features are completed this looks pretty good :) |
|
I myself don't have a strong opinion about the parameter values yet. I think the point above by @alexcjohnson about standardizing lowercase is good, however like you say maybe an exception for acronyms should be made. The class name is indeed a bit long. But I think we shouldn't worry so much about naming conventions yet in new drivers until we agree on a proper standard, after which we can update the existing drivers. |
|
|
||
| def _set_units(self, units): | ||
| for param in [self.X, self.Y, self.R, self.sensitivity]: | ||
| param.units = units |
There was a problem hiding this comment.
This does not seem to update the units of that parameter, at least when it is remote, or does that work for you?
refactor: made those conversion tables (constant) class attributes
- Capitalized acronyms - Change current input config options
|
@MerlinSmiles I changed the input config options for current measurement. What did you mean exactly with the remote parameters, I'm not familiar with those yet. |
|
@Rubenknex Thats much clearer, maybe we should add docstrings to the parameters where it makes sense to have some? like the input config. The remote parameters are a consequence of multiprocessing, there the instrument itself lives in a separate process, and all communication goes through queues, so just setting |
|
@MerlinSmiles that's right, I hadn't considered that these attributes could change when I wrote the remote stuff... We should change them into properties using getattr and setattr so the remote doesn't hold their state at all. |
|
@MerlinSmiles So a parameter docstring is only meant to be used when a parameter requires some further explanation? Because I saw one instrument driver where most parameters had a docstring, but this would be like copying the instrument manual. |
|
@Rubenknex actually I dont know. I think there is no need for a docstring when the parameter makes sense by itself, but I'm sure @giulioungaretti will yell at me for that statement. I think it makes sense when there are parameters with a few options, so one can check for the possibilities. |
|
@MerlinSmiles Yes I agree with that. If the parameter docstrings will be used in the auto generated documentation of qcodes then I think it will be useful to people when writing measurement scripts. |
|
@Rubenknex I'm sure that will happen. But also from an interactive environment to do a |
|
But then we also have a problem with updating the docstring when changing the meaning of a parameter, @alexcjohnson |
|
I would think the docstring should describe all the possible behaviors of the parameter. It will be useful to people to see when a parameter has the possibility of changing in response to another one. That is also the only way this can be compatible with auto-generated docs - although I suspect this will be hard for drivers, you basically have to instantiate one before you can figure out what parameters even exist. It will be nearly impossible just by parsing the code, particularly in cases like making parameters inside a loop. |
|
Hm, so how should we deal with this? |
|
We'll still get all the regular docstrings, so it'll be easy to see all of the static docs. It's just Parameters and Functions that will be tricky. I'm sure we'll come up with a solution at some point, but at least initially it may involve things like documenting that there's a parameter named Actually, this connects with another issue, making some sort of tests instrument drivers can run offline - so we avoid problems like #202 |
|
@MerlinSmiles not sure I understand this comment: https://github.com/qdev-dk/Qcodes/pull/139#issuecomment-227123101 |
|
@giulioungaretti when the meaning of sensitivity chages from volts to amps the docstring needs to be updated too, no? Not sure how that works with #244 though. |
As I said above I would argue the docstring would be more useful if it described all possible behaviors, regardless of what it's doing right now. So then it doesn't need to change. |
|
@alexcjohnson , @giulioungaretti
How about supporting both? The default docstring explains both options but also shows which one is currently selected. I feel that having the current one in there is quite important for usability, however I also feel that that is a different issue unrelated to the quality of this driver. |
Yes, I can see that. This would work fine for local instruments, but unfortunately it's not so easy for remote ones. For annoying reasons that I can't seem to get around, we can't have the remote query the server every time the docstring is requested, so we would have to have the remote somehow know, from the server, when there has been a change to the docstring. I described this problem a bit in https://github.com/qdev-dk/Qcodes/pull/244#issuecomment-228189701 Anyway, quite right that this is a separate issue. What are we still waiting for to merge this? I don't think we need feature completeness, future PRs can add in missing pieces, but we should make sure features that are here aren't broken. Also naming conventions? I agree that Re: capitalization - for parameter names I would like to follow the python attribute naming convention, which is normally |
|
As far as code goes I 💃 this! merge conflicts are now fixed. |
|
Just one more quick discussion on naming - since the folder name here conflicts with #190 where @MerlinSmiles has a driver for SR560. What Merlin did is: |
|
Looks good, I prefer the shorter name as well. To confirm this would make On Thu, 30 Jun 2016 at 18:24 alexcjohnson notifications@github.com wrote:
|
Exactly. |
|
@spauka @alexcjohnson cool, then I will just go ahead and make this change |
Reflect the model number/name on front panel and simplify the path.
|
merging now, later an or few issues will be made to summarise some of the points that came up! |
6 participants
Instrument driver for the Stanford Research Systems SRS830 Lockin Amplifier
There are still a couple of todo's left, however at this point it is able to take data.
Still remaining is:
@alexcjohnson
I wrote a new validator for boolean-type values, it seems like this sort of parameter is generic enough to warrant a new validator. It may also make sense to create a getter/setter shared between instruments as on/off type values will occur often. Whether or not this is the best way of handling it, we should come up with a guideline for handling this sort of parameter.