-
Notifications
You must be signed in to change notification settings - Fork 318
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
Switching to QCodes alpha/beta version #8
Comments
Thanks @AdriaanRol ! This is a great list. Let me just address a couple of them right now:
3.3+ is the requirement. I downgraded so people wouldn't be forced to use 3.5, but 3.5 is actually what I have installed for development. But you raise a good larger point:
The sooner you (and others) get started developing within this repo the better, so I'm heartily in favor of a branch, not a fork! That way it's easier for me to see what you're doing, and for us to work together on the same features.
What do you think of my AverageGetter example? This one just returns a single value but it's a parameter like any other so could return various values of arbitrary size. (I'm working now on extending parameters to allow a single parameter to return several different sized outputs) |
Looks great, from what I see it would indeed allow arbitrary code inside the loop. Altough from what I see it saves only the outer value (and not the intermediate data) while quite often we do want to access the intermediate data afterwards. I think the tricky part here would be how to handle the datasaving. If I understand correctly there are two challenges here
I can think of two ways to address the datasaving problem (and once that is clear it should be possible to think of the best way to do point 1). Option a. Flat saving:
This is what we do now, very inelegant and inefficient but easy to implement and easy to extract. Option b. Nested saving :
Keeps all the data corresponding to the same Loop in one file. It would require the dataserver keeping track of which Loop is a "child" (wrong word but cannot think of the correct term) of which outer loop in order to keep the nesting straight. Would also require a way to extract the inner datasets. Option c. Only outer:
Only save the outer loop :( |
Just to be concrete, lets say the nested parameter runs a class MyFitter(Parameter):
names = ('raw', 'y0', 'slope')
labels = ('Raw data (mv)', 'Y intercept (mv)', 'Slope (mv/sec)')
sizes = (200, None, None)
setpoints = (range(200), None, None)
setpoint_names = ('time', None, None)
setpoint_labels = ('Time (sec)', None, None)
def get(self):
loop = Loop(repeat(200), 1).each(self.measured_param)
data_set = loop.run(background=False, data_manager=False, location=False)
time_array = data_set.arrays['time'].data
data_array = data_set.arrays[self.measured_param.name].data
y0, slope = line_fit(x=time_array, y=data_array)
return (data_array, y0, slope) Now lets suppose we repeat the acquisition and fitting ten times like this: data = Loop(c1[1:10:1], 1).each(MyFitter()).run(location='data_and_fit') What I had in mind was not to make a separate data array for each of the internal measurements, but to incorporate them into one higher-dimension output. So this loop would create three output arrays:
How does that sound? To pull out an individual raw measurement you have to slice the raw array. One nice thing about that is it makes the correspondence (which part of the |
So to be clear, this isn't either option a or b that you describe, but all the |
Initially we also wanted to do it as a big array that you slice. However it is not obvious to me how to scale up to higher dimensions/levels of nesting. An additional disadvantage of this method is that it requires you to specify all levels of nesting explicitly in the outer loop (altough some would argue that is an advantage) I think there are several advantages to nesting the data in the same way as nesting the measurement loop.
One a different note, nesting is ofcourse the same as creating a multi-dimensional array on an abstract level. |
@alexcjohnson In order to convert instrument drivers to QCodes I figured it be a good idea to create a tutorial notebook explaining how to make an instrument driver, first based on your MockInstrument class and then using custom parameters. I have now finished the mock instrument creation (see 663cb0d) but am running into a BrokenPipeError with the dataserver when attempting to use it with a Loop. I saw a comment in your example code about having the instrument definitions in a different file. # spawn doesn't like function or class definitions in the interpreter
# session - had to move them to a file.
from toymodel import AModel, MockGates, MockSource, MockMeter Could this be the same thing that is causing my error, and if so do you know what causes it? |
It's possible, did you try moving the class definition to a separate file and importing it? That looks like a slightly different error than I got, but it's still related to pickling which is where it failed before. Definitely an annoying limitation for the sake of making self-contained examples... in actual use there's something nice (for maintenance and reusability) about putting definitions into a separate file, but ideally not something we'd have to force on people. I'll look a bit into whether we can do something to get around it. |
Can you elaborate?
I bet we can solve this by abstracting the concept of slicing an array a little. So, if you have the
Then any analysis you could do with an un-nested measurement can be done with a slice of a nested measurement. |
I haven't had time to look into this yet but so far the instrument drivers I have made seem to be working fine.
Because nested datasaving preserves symmetry between different levels of the data acquisition loop the preexisting analysis can be used on all layers, additionally when creating the loop there is no need to specify variable names and shapes beforehand as you have done in your average getter example. Altough I can imagine a similar automated way can be made for other varieties we can come up with. I think the main advantage of using a hierarchical dataformat to nest is that it is a more general form of slicing. Addressing nested levels of a hierarchical dataformat can work in a very similar way to slicing with the only difference that you are not slicing an array but a datastructure that can contain other types of variables (such as nested dictionaries with metadata or snapshot objects).
I think disallowing taking metadata at nested levels is a bad idea as you throw data away (or at least make it harder to access if you use a logging system.) The reason why I am advocating a hierarchical system is that it is a preexisting method that already provides all the functionality we are looking for. No need to reinvent the wheel. And if you insist on text based datasaving, JSON should be able to handle that just fine :). I think your method can work but that it also requires a lot of code for the datareshaping, something which is hard to do in the most general way possible, makes the code less accessible for collaborators and reduces the modularity. |
simple creation of "parameter holding" parametersWhen converting QTLab instruments I run into a lot of instruments that have simple "holder" parameters. They exist t ensure some parameter of an instrument is logged and viewable. They usually look something like def _do_set_paramname(value):
self.paramname = value
def _do_get_paramname():
return self.paramname To have the same behaviour in QCodes I would have to do the following self.add_parameter('paramname',
get_cmd=self._do_get_paramname,
set_cmd=self._do_set_paramname,
vals=vals.Anything()) Replace the set and get functions with an underscore def _do_set_paramname(value):
self._paramname = value
def _do_get_paramname():
return self._paramname There is probably also another way by creating a parameter from scratch in the init and using that but I haven't looked into that yet. As I very much like the idea of passing strings for instrument driver write commands I thought a similar shortcut for such 'holder' variables would be very nice, something along the lines of self.add_parameter('paramname',
get_cmd=holder_getfunc,
set_cmd=holder_setfunc,
vals=vals.Anything()) I do not know how best to implement how best to implement this as I am not familiar enough with all the under the hood things in QCodes but as it is such a common task I thought it might be a good idea to consider. |
Is there some reason why you can't completely code-gen this (parameter-holding parameters)? Define an "add_simple_parameter" method that just takes the parameter name and vals, and that generates the getter and setter methods and adds them to the object directly? |
"Holder" parameters are an interesting case, and you're right that these will come up all the time. I'm guessing these are mainly for manual settings that the software otherwise has no way of knowing about, like what kind of attenuation, resistors, cables you have installed, or switches/dials that are invisible to software? I'll make a Which brings up the other challenge with these parameters: making sure they stay in sync with reality. The only way I see to promote this is visibility: making sure these parameters are very apparent (and easy to update) in whatever monitoring panel we make. |
You're absolutely right that we shouldn't be dropping functionality just because some technical aspect of a different system forbids it. But I still think we can get everything a hierarchical structure can do and a bit more with a multidimensional structure. The reasons I'm advocating such a structure (which are all interrelated) are 1) it preserves the meaning of the outer loop(s) so you and the software don't have to infer this from the hierarchy, 2) it makes analyzing and visualizing the whole easier and 3) it makes it easy to slice in arbitrary ways, which would otherwise require looping across the hierarchy. I guess what it all comes down to is I think it's easier to slice than to put things back together again - so it puts the pressure on me to demonstrate that the slicing method I propose is as easy as I think it is! I suppose when you talk about nested metadata, you're thinking of monitor data (fridge temperature, for example) that might have been acquired many times during the larger measurement loop? That's a fair point, but it seems to me the right solution is just to make an array of metadata dictionaries that mirrors the structure of the data. That could lead to a lot of redundant storage, if this becomes a problem we can strip out anything that matches the highest-level metadata dictionary, and reconstitute the full set on slicing. |
Some more stuff that came upWhile building drivers I ran into some more issues/questions
self.add_function('reset', get_cmd='*RST?') Cannot be called in a convenient way instrument.reset() # Does not work
instrument.functions['reset']() # is the way the function has to be called instead.
instrument['reset'] # does not work because this refers to an entry in the parameters dict instead Additionally this also breaks terminal completion to see what functions a parameter has.
|
Good point - I delegate wiring this up to tab completion is on my todo list |
Ah OK, I wondered if this would come up... I wanted to enforce a single argument here to maintain the idea that each settable parameter is a single degree of freedom, but I guess with an argument like this you're defining how you set that value, not what you set. Do you think this is going to come up often enough that it's worth breaking this rule (allowing one "value" and arbitrary kwargs, for example)? We can certainly do that, but first lets think about alternatives that would let us keep the rule. If it's only going to happen a few times, and you're not going to need the extra args in measurement loops, it might be better to make the less-common form of the parameter into a function (that can call the original parameter's If you're going to need both forms in measurement loops, the multiple argument issue gets a lot worse (how and where do you provide the extra args? do they need to be saved as part of the setpoints?) That might argue for a bit of a structural change, for example make the extra args into parameter or instrument state variables - like |
Good ideas - these will all be easy adds.
There's a fairly small set of symbols in the main namespace and an even smaller set people will need most of the time... I hate from qcodes import Loop, Task, Wait # no prefix on the stuff you use all the time
import qcodes # full prefix for anything else But that's kind of a matter of taste - I'm happy with |
Visa error handlingWhen I use a visa write command the visa handle returns an error code (0 being no error) (e.g.) In [58]: AWG.visa_handle.write('*IDN?')
Out[58]: (7, <StatusCode.success: 0>) I found that while writing drivers or when testing devices it is quite essential to get a confirmation that my command is executed correctly. The set_cmd gets routed to the self.write_async(cmd) of the VisaInstrument class. This function looks as follows. @asyncio.coroutine
def write_async(self, cmd):
# TODO: lock, async
self.visa_handle.write(cmd) I would propose adding error handling to this function, the most naive way I had in mind would be something like @asyncio.coroutine
def write_async(self, cmd):
# TODO: lock, async
ret_code = self.visa_handle.write(cmd)
self.check_errors(ret_code)
return ret_code #return so that if you should want it you can catch this value. This self.check_errors could be included as a function in the VisaInstrument in which it checks for all standard Visa errors. The function could then be overwritten within the specific instrument classes to include driver specific errors. |
good idea - and the default |
Instrument get allI added the following function to one of my instrument drivers (AWG5014) and thought it might be useful for all instruments. Should I just add this to the instruments.base.Instrument class? Or do we want a different name, or perhaps have it as an option in self.snapshot(), e.g. update=True def get_all(self, update=True):
# Ensures updating
if update:
for par in self.parameters:
self.get(par)
return self.snapshot() |
I like the idea of this being an option to snapshot 👍 |
Question regarding incomplete and unpolished drivers@alexcjohnson Shall I just add them with a notice of what is working and what needs to be improved? |
You're right, we definitely want this incomplete code in QCodes - the only thing I'd be concerned about is locking people into an API that we will want to change once the drivers get cleaned up. If you think the cleanup is going to require breaking changes for users of the early version, there are a few options:
|
Added "status" to the docstring of the two instrument drivers that are added as discussed in issue #8
GNU public licences,As I was adding another driver it occured to me that adding the tektronix driver we are using (based on a heavily modified qtlab driver) has a GNU licence in it. Thought I'd ask before committing it, (note that the other drivers I add either were never part of QTlab (the Signal hound driver) or are a complete rewrite/port by me) |
Just out of curiosity, what is the intended license for Qcodes? |
Ah, good question - by the time this is open sourced we don't want to have the GPL on anything, but for the moment I'd vote for putting it in the repo, maybe just in a distinct folder so we don't forget about it? Then before broad release we can replace it. @alan-geller does that seem like a reasonable strategy? @MerlinSmiles the intended license is MIT or similar, so it could potentially be freely included in commercial products. That at least seems to be the consensus, but it could still be up for discussion. |
The visa instrument classI was working on the IVVI driver yesterday (ee95806) and ran into some limitations of the base instrument class.
Units and labelsI also had some thoughts on separating the label in name and units. I remeber that in issue #6 there was a discussion on separting the label into the name and the units. I could at the time not come up with a fundamental reason why one would want to do that, however I think ther are are two reasons to do so.
|
Indeed! This is stickier than I thought at first - I hoped we could just add
Or, though this is a bit speculative, we could do our own conversion between the two when we find Alternatively, if just a couple of commands need non-ascii bytes, the others could take a string, and in a custom Thoughts? Do you want to give me some more info on what the custom byte-based protocol looks like? |
Sold. we'll put in a separate unit field. Maybe if we're extra clever this will even let us dynamically adjust the label if there are exponents factored out of the tick labels. |
Yes, I guess that will come up occasionally... and more generally, I think we'll see more things that at first seem like one-off quirks that turn out to be repeated across many instruments. That's a nice advantage of the instrument drivers being contained in the same repo, so we can develop the functionality in a particular instrument at first and then generalize it and shift it up to a base class, updating the original instrument at the same time so upgrading is easy. I was going to suggest: But on Windows |
actually that platform-dependent part won't be hard at all: import sys
if sys.platform == 'win32':
# On Windows, the best timer is time.clock
default_timer = time.clock
else:
# On most other platforms the best timer is time.time
default_timer = time.time |
sounds like a good stepping stone before incorporating some behavior in the base package. Just be careful to do it without making closures or lambdas - no functions or classes inside other functions or classes - that's another thing (or maybe the same thing) that "spawn" multiprocessing doesn't like. I transformed some of the closures I had made before finding this out into callable classes qdev-dk-archive@c866552 |
On licenses: I have been assuming MIT, but that's not decided yet as far as I know. I think putting GPL-ed code into another folder that is optional to use (i.e., not pulled in by the main code, but something that the user has to explicitly include themselves) is OK, but I'm no lawyer. Sorry for the delayed response, I was out of town (and mostly off the grid) over the weekend. |
I think once it's open sourced, we'd better have the whole repo on the same license - otherwise people will still get antsy about having that code there, even if they don't use it. At that point if need be we can split these out into a separate repo of "optional drivers." Ideally though we only have a few of these drivers and we can find a time to rewrite them ourselves. |
@alexcjohnson: Yes, definitely, a separate repo would be best. |
In that case I propose the following syntax for adding a get_cmd/set_cmd
Method would be the way to generate all basic and basic multi-channel commands. What do you guys think? |
Better error messagesI am running into some errors with one of my instruments and get the following error.
This happens in the parameter.get() function when I call my get_all() function. This error message itself is quite fine but it would be super convenient if it would additionally tell me for which parameter this error gets raised. |
It's going to be hard to do much in general for messages like this that we don't generate. But do you know about the debugging magic command? Type |
Agree completely on not being able to fix error messages that we do not generate. However this is an error that is generated by QCodes and quite hard to find out where it comes from, as it is in the quite low-level auto-generated parse function. A simple addition of a self.name in the print statement would make it far easier to find out which parameter this bug corresponds to. Especially because this is an error that I expect will be quite common. However I do not know if all objects that will ever use the sync_async functions will neccesarily always have a .name property.
No I did not, I'll check it out |
I don't see anything in |
The error is not in the float function. The driver is correct, however this occurs when I miss a message. The response to the specific parameter should be '0.0\n' which can be converted to a float, however because commands get out of sync (due to some user related mistake) I am reading out the response to the previous query which is something that should get parsed to a string. The issue is not with the error being raised but rather with the ease of debugging it. |
@alexcjohnson as discussed in issue #8.
Auto-completion of parameter namesWorking with QCodes for the last weeks made me realize that there is one feature I miss a lot and that is the tab-completion to quickly see what the functionality is the instrument provides. I noticed that you made I would suggest adding both functions and parameters as attributes to the base instrument object to allow the tab-completion and easy to use commands. This would then get rid of the custom On a separte note, what would be a good merging strategy? I think that the stuff I made is not yet up to release quality but I think it makes sense to keep things together in some sort of development branch in order to prevent us from diverging in development. |
I will first take a look at implementing this with But I agree, I likewise find myself sorely missing autocomplete while working with remote pyqtgraph, where the proxy apparently doesn't map |
What kind of quality issues? I don't think it's a good idea to develop a strategy to allow us to punt code quality to a later time, because it will never happen. If there are specific things missing it's OK to put in I've been punting tests myself, in an effort to get the code in more peoples' hands quickly, but pretty soon I think I'm going to have to set aside a week (or more) just for tests, to catch up here. I'm OK with omitting instrument driver tests for now, we're going to have to develop a coherent strategy around this to do meaningful testing (not just mocking the hell out of everything in mindless pursuit of coverage) without the actual hardware. |
And if there are things you want me to do within your branch prior to merging, I'm happy to. |
Agree defining things doubly is a bad idea, I did not know of the
I think this is a pretty solid answer to the merging strategy. I already have a test-suite running for our homebuilt FPGA box (not in QCodes). I was planning on making a similar test-suite for the RS source because that is the simple example and makes it easy to find out for what other RS models it works. (Don't expect it tomorrow though ;) ) Also what package are you using for unittesting? I currently use unittest but I find that I cannot pass an initialized instrument to it (Ideally a unittest should be completely self-contained but I find that I need to specify some initialization arguments to the instrument such as the address), Instead I use a workaround where I make the instrument object globally available to all tests but that is a bit dirty. |
I have to admit I don't know what nose is, other than a way to run unittest tests 😇 Check out the test directory: I use It might be worthwhile creating a config system for testing against real hardware, then maybe passing in a filename to the tests (a file that's outside the repo), in which we define which instruments to test and what their addresses / other config settings are. |
nose-testconfig as referenced in that SO post looks like a nice way to go... but lets leave this for a little while. |
|
Thanks, there's a few more things I want to do before the pull-request:
The pull request will then contain a set of basic drivers There is also another issue with the way the instrument parameter behaves. If I understand correctly the get-command is always called after a set. This should normally not be a blocking operation as the QCodes loop is constructed such that this can be done asynchronously. |
Is it? It shouldn't be... where are you seeing this? |
Great! Most of those sound pretty quick. I'm trying to balance the desire for small, frequent PRs (which so far I've set a bad precedent for, but I'll try to make them small from now on) with encouraging testing... I think testing wins, but you can always make the PR before writing the tests, so I and others can start looking at it, make a checklist in the PR itself so reviewers know there is more coming, and push tests into that branch when they're ready. |
Close for now, make a new one if things are still relevant <3 ! |
@alexcjohnson
Hi Alex,
When you visited just before Christmas we had some good discussions on QCodes and we agreed that I would provide a list of features that would be required in order for us to start using QCodes.
Essentials for switching
Modifications to Loop() concepts we use that are currently not mapable to QCodes
In addition to these essentials there are quite a few modifications we need to make to our code to transfer over. For instance we need to translate our sweep and detector functions to set-able and get-able parameters, we need to change our analysis to work with changes to the dataformat etc.
Our plan for beta testing is to first transfer over the bare-essentials. This means transferring over basic instrument drivers but continue using our existing measurement control and analysis. The plans are to convert our sweep and detector functions to parameters such that they can be used both by QCodes and our measurement control before switching over completely.
When beta-testing those first features I will probably run into all the little details of features that are nice to have. I will also add those to this issue
Practical notes/features and small misc things
Some random questions
Q: I saw some notes in the code about python 3.3 vs 3.5, which version are you actually using and why (and can I just use 3.5 or do I need 3.3)
Q: Do you prefer if I mess around on a branch of QCodes or if I make a fork when trying out new things (e.g. demonstration notebook )
P.S. I tried to keep this issue short to prevent the whole wall of text thing and to make it a nice short checklist. However it does rely a bit on the discussions we had before christmas. If you (or anyone else who is interested) has any questions, comments etc feel free
The text was updated successfully, but these errors were encountered: