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

Negative Delay #13

Open
damazter opened this issue Mar 12, 2016 · 14 comments
Open

Negative Delay #13

damazter opened this issue Mar 12, 2016 · 14 comments
Labels
enhancement New feature or request

Comments

@damazter
Copy link

This issue might be related to microsoft/Qcodes#44 and microsoft/Qcodes#47
upon writing a numerical simulation as an instrument several problems occurred (described in separate issues)

I want to sweep over some numerical parameter such the energy of the system and measure some transmission property. the loop i wrote is:

loop = qcodes.Loop(system.parameters["energy"][0:1:0.1], delay=0)
data = loop.run(background=False)

where system is the variable containing the numerical simulation instrument
(the background=false is part of a separate issue)
upon running this code i get the same warning as @AdriaanRol got before:
WARNING:root:negative delay -0.000041 sec

In the case of such loops (containing numerical work rather than experiments) I feel that there should be some easy/trivial way to remove this sort of delays from the loop without causing this warning.
I feel that this behavior should be default for the syntaxes:

qcodes.Loop(system.parameters["energy"][0:1:0.1], delay=0)
qcodes.Loop(system.parameters["energy"][0:1:0.1], delay=None)
qcodes.Loop(system.parameters["energy"][0:1:0.1])

I understand that this warning is very important in some loops, but that is only the case if delay>0.
For loops not containing a delay, the actual time taken will always be larger than 0 and the warning is meaningless.

@alexcjohnson
Copy link

That sounds reasonable, 0 or None suppressing delay warnings. I'd rather not allow users to omit delay though - would be too easy to do accidentally, which can sometimes actually break things.

@AdriaanRol
Copy link

which can sometimes actually break things.

Do you have an example of this? I would argue that delay=None should be the default option unless there is a strong argument why that is a really bad idea.

@alexcjohnson
Copy link

It's not going to break things in code... I just mean like ramping certain parameters too fast can break a device.

@akhmerov
Copy link

Shouldn't this rather go into the driver, as opposed to the measurement loop?

@AdriaanRol
Copy link

Should that not be set in the instrument driver itself?
The way we currently operate is that we set the delay settings upon initialisation of our instrument. All this is set in an initialisation script that is very rarely modified and prevents any user mistakes on runtime.

I think the alternative you are proposing, which is setting the delay in the loop is far more error prone, I see it going wrong in two ways, the first is accidently forgetting a zero in the delay you set, the second is people copy pasting and forgetting about it altogether.

@alexcjohnson
Copy link

Ah yes, it could go in the driver via an init script... that's a good idea. I think it's generally not a good idea for this to be part of the drivers themselves, because it will vary by experiment, but if the instrument itself cares requires some delay for safety then I guess it can be put in the driver. Or for simulations where you know delays are useless they can be preset to zero. How about this:

  • each parameter can have a default_delay attribute that gets used if the Loop call does not specify a delay or specifies None
  • therefore, None is NOT no delay, you must specify delay=0
  • if neither the parameter nor the Loop call specifies a delay, it's an error - ie there's no global default, zero or otherwise.
  • we will need to allow delay to be a function of (old_value, new_value) for this to make sense as part of the parameter in a lot of cases (we talked about this at some point but I don't see it now).

@AdriaanRol
Copy link

@alexcjohnson

I would like to prevent introducing to much new complexity. I think there are two reasons why you would want to specify a delay. 1. Device/experiment safety reasons and 2. "Experiment" loop specific considerations.

For option 1 we have a perfectly fine way of specifying this in the instrument driver, the enhancement I would propose would be the ability to change this either upon initialization or after initialization of an instrument. For option 2 it belongs in the loop, however zero (0) is the sensible default if it is not specified.

Adding a new default_delay parameter does not make sense to me. It only convolutes the amount of code to specfy otherwise trivial measurements.

To summarize I propose the following:

  1. Make parameter delay as it is currently implemented changable after initialization (I think we already can)
  2. Make 0 the default delay for Loop

@alexcjohnson
Copy link

@AdriaanRol

For option 1 we have a perfectly fine way of specifying this in the instrument driver

you mean sweep_step and sweep_delay? Yes, that's true, good point. As it stands now, sweep_delay only applies if you also have a sweep_step, but we could apply it even without a step, then it's just the delay after any set call. Perhaps then it would be clearer if we changed the names to set_step and set_delay?

and re: changing these after initialization - yes, there's a set_sweep method but in light of this discussion we likely want to rename it and let it change just what you specify to change, right now I think it requires all arguments.

@MerlinSmiles
Copy link

@alexcjohnson is there somehow a fix for this? Or anything I can do until there is a real fix?
These negative delays flood my notebooks and its basically unusable 😢

@damazter
Copy link
Author

Same here, printing the warning statements seems to be the major bottleneck for speed of my notebooks.

@alexcjohnson
Copy link

Almost ready with a PR on this...

@alexcjohnson
Copy link

@MerlinSmiles @damazter this should be resolved by microsoft/Qcodes#116 - closing, but feel free to reopen if issues persist.

@MerlinSmiles
Copy link

@alexcjohnson I still see these negative delays, during my measurements. I guess I didnt see this for a long time because I removed that warning in my local copy 👼

@giulioungaretti
Copy link
Member

negative delays still happen and it's totally not clear what they mean IMHO.

@jenshnielsen jenshnielsen transferred this issue from microsoft/Qcodes Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants