Skip to content
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

Docs/params #403

Merged
merged 18 commits into from
Jan 19, 2017
Merged

Docs/params #403

merged 18 commits into from
Jan 19, 2017

Conversation

alexcjohnson
Copy link
Contributor

@giulioungaretti I still need to write the example notebooks (and there are a couple of TODOs scattered about here) but for discussion purposes, here's the Parameter refactor I've been working on.

  • Parameter is now only for single-valued parameters

  • the new MultiParameter has only one constructor form, and is for all multi-valued parameters

    • One of the TODOs that I think people would really appreciate is to make this settable, perhaps the easiest way to do this is to mimic combine? I haven't looked at this though.
  • StandardParameter and ManualParameter are unchanged.

@MerlinSmiles
Copy link
Contributor

I still think it should be unit and not units for the single valued parameter... 😄

@nataliejpg
Copy link
Contributor

Could you clarify what 'multi-valued paramaters' are? Does this include both the hardware controlled arraylike parameters (which you don't set but could be described as having a multi-valued return) and for example b field parameter with x, y and z values?

@alexcjohnson
Copy link
Contributor Author

Does this include both the hardware controlled arraylike parameters (which you don't set but could be described as having a multi-valued return) and for example b field parameter with x, y and z values?

Yes. Anything whose values are more complicated than a single number (or string).

We may decide that's an oversimplification - which is part of why I posted this now rather than waiting until I finish the example notebooks. Basically, what we had before was this horrible Parameter constructor with 5 cases to choose from and different behavior of the arguments in each case. I realized that in terms of functionality we could reduce it to 2 cases (the old case 1 and case 5), and break them into separate classes, because the simple case makes it easy to make simple parameters, and the most complicated case encompasses all the others.

So far the only real limitation I see that this imposes is that if you want to return a single array ([1, 2, 3] or numpy equivalent), you need to wrap it in an extra layer ([[1, 2, 3]]) because it's specified in a form that can return multiple arrays. I suppose that could be confusing if you want to manually look at the result of param.get()... If folks think that's too cumbersome, I could certainly put back case 3, either as an optional feature of Parameter (specify shape, and the setpoint info) or as a separate class like ArrayParameter.

@AdriaanRol
Copy link
Contributor

@alexcjohnson , I don't really understand the motivation for introducing a new type of parameter. I think that the current parameter is very capable of providing exactly this interface, and furthermore is already used in this way. As far as I understand the problem, the only real problem at this point is the way it interacts with the Loop (which if I understand correctly has been solved recently).

Could you clarify what 'multi-valued paramaters' are? Does this include both the hardware controlled arraylike parameters

I have this question as well. Looking at the code suggests this is the multi-valued parameter and not the array parameter.

I still think it should be unit and not units for the single valued parameter...

@MerlinSmiles I disagree, having a single attribute units would allow working with an interface that does not care about what kind of parameter it is. Even if the list/tuple of units contains only one element. This was as far as I know the motivation for naming this attribute units initially.

@MerlinSmiles
Copy link
Contributor

@AdriaanRol Its the first time i hear that argument, but that does/did not hold for labels names and shapes then?
To my knowledge it was grammar aesthetics that led to this back then.

@AdriaanRol
Copy link
Contributor

it should also hold for labels, name and names is debatable as name refers to the name of the parameter but is used as a short for the labels whenever labels is not specified.

Shape should still be a single one as it describes the shape, wether it is a tuple based (multi-valued) and/or array based.

@alexcjohnson
Copy link
Contributor Author

@AdriaanRol you're right that the current parameter is fully capable - this doesn't add any functionality, it just reorganizes in an attempt to make it easier to understand and document. All kinds of parameter have been and continue to be supported as measurements in loops, and it's on my/our TODO to support multivalued set within the parameter without having to make separate parameters and combine them.

having a single attribute units would allow working with an interface that does not care about what kind of parameter it is. Even if the list/tuple of units contains only one element

This is actually exactly Merlin's argument... one which he and I have gone round on a few times before 💫 - in the simple case, units is a string, and in the multi case units is a sequence of strings. I suppose you've pointed at one way to resolve it, which would be to always make units be a sequence, though that seems like it would be confusing in the simple case. No, my argument initially is that you pretty much always speak of units in the plural even for a single measurement.

@alexcjohnson
Copy link
Contributor Author

re: name(s), label(s), shape(s):

For the purpose of this discussion "multi-valued" means a parameter that provides values to go in multiple DataArrays - so NOT if we reinstate case 3, a parameter returning a single array value.

  • all parameters need a name. A multi-valued parameter also needs names, a sequence of strings to give names to each array.

  • single valued parameters have label, multi-valued parameters have labels, a sequence of strings.

  • multi-valued parameters have shapes, a sequence of shape tuples. There is no shape unless we reinstate case 3.

So from a consistency standpoint, unit and units is correct, but from a standpoint of "make simple things simple," I feel like I'd always be messing up and typing units in the simple case because that's what I always think of it being called.

@MerlinSmiles
Copy link
Contributor

When I look at the wikipedia use of unit: https://en.wikipedia.org/wiki/Units_of_measurement it is like how I would do it. A single value has a unit of measurement, a list of values may have different units of measurement.
I can turn your argument around and personally always make the mistake to ask for the unit of a single value while i should be asking for the units of that single value.

Another question I have, is the multi valued parameter in principle not just a list of parameters? and combined each name goes into a list of names.
The reason im asking: Can I just split and combine multi-valued parameters? param = multi([par1, par2, par3])

@alexcjohnson
Copy link
Contributor Author

When I look at the wikipedia use of unit: https://en.wikipedia.org/wiki/Units_of_measurement it is like how I would do it.

Interesting... not how I use the word, but who can argue with wikipedia 🙉 If others feel it would be less confusing to use unit for a single value we can change it.

The reason im asking: Can I just split and combine multi-valued parameters? param = multi([par1, par2, par3])

That's what combine is getting at, but MultiParameter is for cases where the whole grouping comes as a single package and cannot be broken into separate parameters.

@alexcjohnson
Copy link
Contributor Author

Update after a chat with @giulioungaretti and @alan-geller - I'm planning to make two changes:

  • You win, @MerlinSmiles - we'll use unit for a single string and units for a list of strings. I'm not going to change all the drivers that use units, I'll keep it backward compatible, accepting units for a single string but logging a deprecation warning. In a few months/versions, after everyone has had a chance to be annoyed by these warnings and update their drivers, we'll remove the compatibility.

  • I'll re-add the single array case, but for clarity I'll make it yet another separate class, ArrayParameter.

More thoughts?

@AdriaanRol
Copy link
Contributor

@alexcjohnson

I'll re-add the single array case, but for clarity I'll make it yet another separate class, ArrayParameter.
More thoughts?

By analogy the Multi Array will be a very common parameter type (think I and Q quadratures of or transmission coefficients (S11, S12 etc) of a spectrum analyzer).

Will you solve this by adding yet another type of parameter? I'm still puzzled by the motivation for these types if beside the naming there is no different functionality.

I think unit/units is a good improvement as long as single valued parameters support units as an attribute which is than a length 1 list containing unit. (and analogously for name(s) and label(s)) .

@alexcjohnson
Copy link
Contributor Author

@AdriaanRol

By analogy the Multi Array will be a very common parameter type

This is covered by MultiParameter - that's for parameters that return an arbitrary number of arbitrarily-shaped return values.

I'm still puzzled by the motivation

It's really just for clarity and robustness - the existing Parameter just has so many arguments and use cases collected in one class that it's very hard to learn, document, and test. I wrote the thing, and I still felt like I had to look through it every time I wanted to do something nontrivial with it.

I think unit/units is a good improvement as long as single valued parameters support units as an attribute which is than a length 1 list containing unit. (and analogously for name(s) and label(s)) .

I suppose I could make these @property aliases in the simpler ones, though I think that would require some changes in Loop, which currently duck types using the existence of names to tell what kind of data to expect. The code in Loop would get a lot cleaner if we decided to not allow duck typing, ie you can't use your own parameter base classes, you have to use one of the classes defined here. Probably nobody would do that right now anyway since it's not documented how you would do it, but I wanted to hold open that option since for single-valued parameters it should be a fairly simple interface.

Anyway any interface that's trying to handle every parameter type will still need some way to know whether get() returns a single item (value or array) or a sequence of items so it seems like a mistake to make them look too similar if their most important feature (what you get from get()) has this basic difference.

@AdriaanRol
Copy link
Contributor

@alexcjohnson , I think the difference between an ArrayLike parameter and a ValueLike parameter is much more important for the way a Loop functions than the distinction between SingleValued and MultiValued (excuse my abuse of naming).

In any case I think I understand the arguments a bit better. 👍 on fixing these issues.

On a sidenote, we now have a pretty clear example ( I think) of all the different loop modes that we implemented in PycQED. If you ever want to take a look at it check out this example notebook. In the end we are solving the exact same problems there. I also still intend to replace our sweep/detector functions completely with the parameters (I've now only partially done this).

@nataliejpg
Copy link
Contributor

I am pro factoring the parameter into several more well defined versions which can then each be more understandably documented rather than trying to explain everything in a really general way as it is done now. I think that will also add some clarity between 'multi valued' and 'arraylike' as they are likely to be used and needed in different scenarios (or at least the 'multi' vs 'array' properties will be even if you want a parameter with both) so its good to define the differences and what functionality each has.

@giulioungaretti
Copy link
Contributor

giulioungaretti commented Dec 5, 2016

Since the PR is far from complete, with docs being highest priority I won't comment much.
But parameters being split into things that actually represent what they are rather than an un-tamable giant class, is the point and probably more important than all the rest. @AdriaanRol tbh, it's really hard to read that example, it's clean code but it's almost impossible to tell what you are doing (but it is interesting, so let's go over that in some other ways).

Copy link
Contributor

@giulioungaretti giulioungaretti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how ABC or duck-typing requirements would make it nicer? What is your idea here, it's really intriguing 👍 I guess to enforce behavior on subclasses and then just take the behavior for granted everywhere-else in the loop ooor?

@alexcjohnson
Copy link
Contributor Author

Not sure how ABC or duck-typing requirements would make it nicer? What is your idea here, it's really intriguing 👍 I guess to enforce behavior on subclasses and then just take the behavior for granted everywhere-else in the loop ooor?

Yes exactly - it's crucial if anyone wanted to make their own parameter classes that do not inherit from one of these - not sure why but who knows what will come up that we haven't thought of yet. But even if everyone only uses the classes we've defined to construct their parameters, subclassing Parameter, ArrayParameter, or MultiParameter still has some requirements this could help them understand. But also for core development, documenting what parameters provide so it's clear how Loop should use them.

@giulioungaretti
Copy link
Contributor

interesting! But first, documentation ! 👾

@alexcjohnson
Copy link
Contributor Author

@giulioungaretti I can add the extra units behavior, but it's a new feature, not a regression, so because this PR is already quite large I'd rather get this merged and then tackle that separately. I already had to overwrite some work @jenshnielsen did cleaning up the docs that I've completely rewritten, don't want to waste any more of your guys' time that way.

@giulioungaretti
Copy link
Contributor

@alexcjohnson cool, yes. Good idea, so we can just focus on fixing this and the data_set and you can go on with the documentation!

We just need to check if we are breaking any instrument.

@jenshnielsen
Copy link
Collaborator

I pushed a fix to the ZNB20 driver, as far as I can see this is currently the only one using a Multiparameter like parameter

@nataliejpg
Copy link
Contributor

@jenshnielsen also most of the alazar acq controller parameters are Multiparameter but I guess that since that isn't merged it's still my problem :/

@jenshnielsen
Copy link
Collaborator

@nataliejpg I'm happy to change it but I should probably do that on the other branch

@nataliejpg
Copy link
Contributor

@jenshnielsen tbh it's quite hacky at the moment so I was going to tidy up a bit anyway in the next few days but it would be great to get some input on the nicest way to do that. I'll bother you on slack... ;)

@jenshnielsen jenshnielsen mentioned this pull request Jan 11, 2017
@jenshnielsen
Copy link
Collaborator

Before merging this I think we should update #435 and #410 to use the new api. Those are useful test cases of the api as the instruments are somewhat more complex

@MerlinSmiles
Copy link
Contributor

It would be really nice with some real life examples of when one would use which parameter.
That would include relations to real instruments, what exactly they could return and how that is organized in the parameter...

@nataliejpg
Copy link
Contributor

@MerlinSmiles I cannot express enough enthusiasm for that idea (note that I have tried though).

@jenshnielsen
Copy link
Collaborator

@MerlinSmiles Agreed, trying to bootstrap that by using it on the drivers that we are currently doing

@jenshnielsen
Copy link
Collaborator

I think this is ok to merge.

Once we revisit data_set and loop we should make it possible to change a setpoint array.
Preferably from the get command of the parameter.

I.e when you call get on a scope trace you will get a preamble from which you can calculate the time axis and number of points -> shape and setpoints. This is the easiest and most logical place to calculate the setpoints but is currently not possible because the setpoints have already been copied to the data_array by containers.

Other todos

  • Examples of real usage TPS2012 (array parameter), ZNB VNA (Multiparameter), AWG?
  • Alazar driver

@giulioungaretti
Copy link
Contributor

The documentation is far from helpful for beginners (waaaay to abstract) I think ,but we'll make a better one.
Many things will still change once we get around the data_set renovation.

@jenshnielsen jenshnielsen merged commit 30a9c3d into microsoft:master Jan 19, 2017
giulioungaretti pushed a commit that referenced this pull request Jan 19, 2017
Merge: f3324a5 9deaec1
Author: Jens Hedegaard Nielsen <[email protected]>

    Merge pull request #403 from alexcjohnson/docs/params
@giulioungaretti giulioungaretti mentioned this pull request Feb 3, 2017
@jenshnielsen jenshnielsen mentioned this pull request Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants