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

Improve snapshotting #759

Closed
jenshnielsen opened this issue Sep 25, 2017 · 11 comments
Closed

Improve snapshotting #759

jenshnielsen opened this issue Sep 25, 2017 · 11 comments

Comments

@jenshnielsen
Copy link
Collaborator

As discussed in #758

Currently all parameters are snapshotted. As suggested by @AdriaanRol there should probably be a good way to turn off snapshotting per parameter without subclassing and perhaps snapshotting should be off by default for array and multiparameters. This again raises the question if Array/MultiParamers are really parameters at all

@WilliamHPNielsen
Copy link
Contributor

Hear, hear! Let's not snapshot them and let's not pretend that they are parameters.

@AdriaanRol
Copy link
Contributor

AdriaanRol commented Sep 25, 2017

@WilliamHPNielsen Arrays are most definitely parameters!

To give some concrete example: we use certain Lookuptable based devices where a parameter of the device is the content of one of these lookuptables. These are simple 1D arrays of varying size.
I see no reason why that should not be a parameter.

@WilliamHPNielsen
Copy link
Contributor

@AdriaanRol, granted, that sounds like a parameter. But the stuff that currently goes into a qc.Loop to be measured is not. I am thinking about: oscilloscope traces, lock-in amplifier buffers, spectrum analyzer sweeps, ADC cards records. None of these represent the state of an instrument in a meaningful way.

@AdriaanRol
Copy link
Contributor

@WilliamHPNielsen To be clear, we are using regular StandardParameters (sometimes Manual) with Arrays type validators for these kind of parameters. To be honest I have never really understood how the ArrayParameter is different from regular parameters but I imagine that is because we don't run into these problems because we don't use the qc.loop.

@WilliamHPNielsen
Copy link
Contributor

To be honest I have never really understood how the ArrayParameter is different from regular parameters

That is a longer discussion that will inevitably lead us into how we need to do a major rewiring of QCoDeS. For now, let me just say that this rewiring is on its way, and that we are thinking a lot about it here in Copenhagen.

@AdriaanRol
Copy link
Contributor

@jenshnielsen @WilliamHPNielsen I just updated to 0.1.7 for the option to exclude some parameters from the snapshot.

However, I find that even though it is no longer queried it is still included in my snapshot. IMO it these kind of parameters should be excluded from the snapshot completely, as the main reason to omit them is to save time when running an experiment.

This is ofcourse quite easy to implement (given the recent changes to snapshot_base). However I was wondering if you consider the behaviour I describe intended behaviour.

@jenshnielsen
Copy link
Collaborator Author

jenshnielsen commented Sep 28, 2017

@AdriaanRol

I think there are two different use cases, of which we only really fix one (Why this issue exists)

The QDac has 48 channels we like to include these in the snapshot but updating them individually is slow. This therefore allows us to skip the update of them individually and just call the get all parameters function once, thus reducing the time to update by a factor of 1/(3*48) (3 parameters per channel)

The other use case is what you describe, I fully agree that should be solved in a better way.
There are many parameters especially Array and mulitparameters that makes no sense to snapshot. We should skip those in the snapshot in a simpler way and they should not (As I guess yours does now) show up with a value of None or whatever the last measurement value was.

@jenshnielsen
Copy link
Collaborator Author

In other works we should push for a fix for this. Lets do that as soon as #600 lands

AdriaanRol pushed a commit to DiCarloLab-Delft/Qcodes that referenced this issue Nov 5, 2017
@WilliamHPNielsen
Copy link
Contributor

WilliamHPNielsen commented Apr 6, 2018

I'm fairly sure #651 resolved this issue's original concern by introducing snapshot_value. @jenshnielsen?

@astafan8
Copy link
Contributor

@WilliamHPNielsen and @jenshnielsen - can you confirm that the issue is resolved via #651?

@jenshnielsen
Copy link
Collaborator Author

I think so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants