labscript-suite / labscript

The 𝗹𝗮𝗯𝘀𝗰𝗿𝗶𝗽𝘁 library provides a translation from expressive Python code to low-level hardware instructions.
http://labscriptsuite.org
BSD 2-Clause "Simplified" License
9 stars 51 forks source link

limits should be stored in the connection table #44

Open philipstarkey opened 6 years ago

philipstarkey commented 6 years ago

Original report (archived issue) by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).


Currently limits only protect you from breaking something when running a shot, but are not respected by BLACS since they aren't saved anywhere.

Device limits (really only relevant to analog quantities and derivatives) should be stored somewhere so that BLACS can access them and use them to protect devices. I'm not sure that connection table properties is really the best idea (should probably be device properties) but since we don't have device properties working for channels (they only work for devices) then the connection table is really the only place left to us.

We should probably also document them (at least show an example in example.py) as probably most people reading this won't even have known limits exist!

philipstarkey commented 6 years ago

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).


I'm in favor of this change but I see one problem with storing limits in connection table properties. The limits of a experiment file need to be the exact same as those of the connection table or connection table comparison fails. It would make more sense if any limits that are within the limits of the connection table would be allowed. So experiment limits (2, 3) with connection table limits of (1, 5) should pass comparison but experiment limits of (2, 6) with connection table limits of (1, 5) should fail.

This could be done by checking them separately in Connection.compare_to. Is this the way to go or does anyone have a better idea? I'd be happy to make the 2 pull requests if there should be no other suggestions on how to get around this.

philipstarkey commented 6 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).


Unfortunately I don't have a better idea. I don't particularly like having to treat "limits" as a special key of the connection table properties dictionary, but don't see any other option other than not allowing you to have limits that differ from the connection table.

I'm not sure...maybe you shouldn't be able to set limits that are different to the main connection table? What are the scenarios where you might want different limits set?

philipstarkey commented 6 years ago

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).


Don't have any example handy, but I would feel better with allowing subranges. This prevents the user from recompiling the connection table in some cases and that always a good thing. I'd consider the connection table limits to be the values to not be exceeded to save connected hardware failure or damage. In a experiment script however one could try to not exceed a sublimit (for some obscure reason) and the compiler could help do that.

philipstarkey commented 6 years ago

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).


Another idea I have is that we could encode the method of comparison inside the key of the JSON dictionary. This is similar to the way you define SQL clauses using Pythonic syntax in Django. For example, instead of saving it as 'limits': [0, 10] we could save it as 'limits__allow_interval_subset': [0,10] to indicate the values that are a subset are allowed during connection table comparisons.

Not sure if I like this a 100%, but could be preferable to hardcoding a special case for limits since it's a) more general (we update the connection table comparison to handle __allow_interval_subset for any parameter in the connection table properties) and b) contains information about the need to do a special comparison in the connection table itself (so it is self-documenting, which is a key principle of the labscript suite).

Note: I realise __allow_interval_subset is an awful suffix, it was just a descriptive example and not necessarily what I would want it to be!

philipstarkey commented 6 years ago

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).


While that sounds more elegant it is equivalent in my opinion. I don't think there is any other case where we store a 2-tuple in the connection table properties that would need this subset feature. Also limits allowing subsets doesn't really need documenting as that is expected behavior. So I'd opt for making limits special and checking it separately in compare to.