m-labs / artiq

A leading-edge control system for quantum information experiments
https://m-labs.hk/artiq
GNU Lesser General Public License v3.0
434 stars 201 forks source link

Sayma SAWG frequency0.set() no bounds checking #1045

Closed jbqubit closed 6 years ago

jbqubit commented 6 years ago
sawg.frequency0.set(1000*MHz)

Produces no error though the frequency is out of bounds. And the output is nonsense.... time-varying amplitude fixed frequency.

sbourdeauducq commented 6 years ago

Bounds checking tends to reduce programming speed.

sbourdeauducq commented 6 years ago

Other drivers don't have bounds checking either for this reason.

whitequark commented 6 years ago

How about having a "debug mode" with bounds checking and "release mode" without it? Python even already has it, it's called the assert statement, and it's turned off if you pass -O or set PYTHONOPTIMIZE=1. As a bonus this will make the compiler faster (though IIRC I didn't use any particularly expensive asserts).

sbourdeauducq commented 6 years ago

Enabling it will often cause experiments not to run due to underflows, so that's not very useful.

hartytp commented 6 years ago

I don't think that a debug mode with different (slower) timings to the "release" mode would be particularly useful.

If one really wanted bounds checking, a better way might be to agree that no set_x_mu functions have bounds checking, but all x_to_mu do. The argument here being that the x_to_mu functions are already generally slow due to FP operations, so bounds checking won't make a noticeable difference.

Having said that, I wouldn't bother; it's quite a bit of extra work (=funding) to add bounds checking to all those functions and it ends up lulling users into a false sense of security -- ARTIQ is quite low level and there are many ways of subtly breaking things, which users just have to be aware of.

Edit: particularly in the context of Sayma, this seems like a distraction from the important business of getting it to work reliably.

sbourdeauducq commented 6 years ago

As a bonus this will make the compiler faster (though IIRC I didn't use any particularly expensive asserts).

If that's a low-hanging fruit to increase compilation speed, then we should add -O to worker invokations.

hartytp commented 6 years ago

Actually, I take that back: lack of bounds checking in ARTIQ is quite an annoying issue, which negatively affects the user experience (can be an easy way of wasting time with obscure bugs).

If we only add bounds checking to SI to mu conversion functions (and hence to SI set methods which call them) then the small speed penalty should be acceptable (these functions are generally not called when speed is required).

Obviously, this is a larger issue than SAWG. It's also much lower priority than getting Sayma to run reliably.

jbqubit commented 6 years ago

Amen! As @hartytp suggests I think this would greatly improve the user experience.

jbqubit commented 5 years ago

This wouldn't be so slow with eg Zynq. https://github.com/m-labs/artiq/issues/1167#issuecomment-427296475