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

Minimal working connection table and example experiment script #54

Merged

Conversation

rpanderson
Copy link
Member

@rpanderson rpanderson commented Jun 10, 2020

Include working connection table script and experiment script for the example_apparatus (see #53), for whole-suite works-out-of-the-box changes. This change see blacs start without raising an exception upon a fresh install (!) with no additional changes, but will require runmanager to load this experiment script automatically.

While not entirely minimal, it demonstrates some basic precepts like child devices and add_time_marker. For this to be a little more meaningful, however, DummyIntermediateDevice should probably:

  • Display digital out(s) controls on its blacs tab.
  • Have a runviewer parser (instructions are currently not displayed.

I am working on incrementally more complex examples that don't require any physical hardware to run (including RemoteBLACS and IMAQdxCamera with mock=True), that extend these scripts.

@rpanderson
Copy link
Member Author

We should consider where other example connection table and experiment scripts will live. These examples are in labscript-suite\userlib\labscriptlib\example_apparatus and are obviously packaged with labscript-utils. This is certainty an option for other examples, or we could choose not to package these and instead have them in a separate folder/repository.

@rpanderson
Copy link
Member Author

We should consider where other example connection table and experiment scripts will live

Another consideration is how users will get updates to the examples connection table, experiment, and analysis scripts. I suppose updating either a regular or developer installation would require copying the example scripts from the labscript-utils installation location to their <labscript-suite-profile>/userlib.

@chrisjbillington
Copy link
Member

chrisjbillington commented Jun 21, 2020

Thanks for the reminder about this in the form of the review request.

I am partial to the structure of the connection table ending in:

if __name__ == '__main__':
    start()
    stop(1.0)

and the experiment script doing:

from labscript_utils import import_or_reload
import_or_reload('labscriptlib.example_apparatus.connection_table')

instead of reproducing the subset of the connection table that it is using. I understand this breaks the idea of the experiment script being self contained, but the shot file still does contain the connection_table.py if one is doing forensics on what connection table a shot was using, and this is in practice what we've been using at NIST and UMD. IMHO avoiding the repetition of the connection table wins out over any benefit of repeating it (which I have yet to wish I had done in actual use).

I'm happy to merge this regardless, but if you agree with me or don't feel strongly, you might consider changing the example to use this structure!

@rpanderson
Copy link
Member Author

Thanks, @chrisjbillington. Relevant to this is the discussion on #52, namely @philipstarkey's comment.

@chrisjbillington
Copy link
Member

chrisjbillington commented Jun 21, 2020

There are definitely pros and cons. Phil's comment about using globals to set device properties is a pretty good point - at some point I would like to make it so that you change device properties using via methods instead of as instantiation args, but this is not currently doable except in a roundabout way.

But I definitely lean toward convenience on this matter - duplicating the connection table is a big cost that is error prone and it's not obvious why you have to do it if you're not setting device properties on a per-shot basis. I don't think being able to recompile an old shot to view it in runviewer is much of an advantage - including its own connection table is one thing, but it still needs globals which are just as likely to have changed in an incompatible way unless you backup old globals files and know which to use. More common is viewing the old shot file in runviewer without recompiling it.

The errors being during compilation instead of upon submission to BLACS doesn't seem like an advantage or disadvantage either way to be honest.

One actual downside we have seen in practice is devices that are present but not in use by a shot. Sometimes a device is broken and BLACS won't run shots. Users in UMD/NIST labs tend to comment the devices out of the master connection table and recompile. This can lead to people commenting and uncommenting and recompiling on a daily basis. Whilst this is a bit messy, It still beats having to synchronise changes in the master connection table to multiple different experiment scripts.

Perhaps the inconvenience of non-shared connection tables is unique to (or exacerbated by) the fact that in our group people sharing a lab are generally running their own experiments independently from other lab users, and changes in the master connection table would require explicit communication with other lab members (who are not present in the lab) so that they update their individual connection tables before they next run shots. Whereas changing a master connection table requires communication so that others know what happened, but no actual action by other lab users.

This does mean that changes to the master connection table are often done by commenting/uncommenting so the previous state can be reverted if it causes problems, so that is a bit messy (a mix of this and version control, but I often see hesitance to use version control for reverting state - it is more used as a log of previous state). The changes are usually incompatible with the previous connection table such that the other lab users would not simply be able to continue with their own connection table without also reverting the lab connection table. So updating the individual connection tables is just this chore that adds no benefit (though it would if device properties were being set on a per-shot basis - this has not been the case so far in the labs here that I know of)

That's just me brain-dumping, doesn't have to apply to this PR. Feel free to chime in further @philipstarkey.

Whilst I'm happy to merge without switching to an imported connection table, perhaps you could put the start()/stop() calls within an if __name__ == '__main__' block such that the example connection table is still importable, even if the example doesn't import it. This would set a good example for anyone who is importing their connection table, and doesn't change anything if they are not.

I will push harder on recommending importing your connection table once setting device properties post-instantiation is more sensible. That's the main impediment I see to it being almost always the better approach.

@philipstarkey
Copy link
Member

The errors being during compilation instead of upon submission to BLACS doesn't seem like an advantage or disadvantage either way to be honest.

If more than one thing has changed in the imported connection table, you have to fix them one by one and try and compile a new shot each time right (instead of getting all the information in one go)?

Presumably you also either need to rewire the lab (back to how it was) or adapt your experiment logic to handle these changes too, no?

I'm not saying people shouldn't do it, but it just doesn't seem like best practice. For a best practice scenario, your experiment connection table should be a subset of the BLACS connection table (only containing devices/channels you are using). You shouldn't have to go digging through compiled shots to see what the connection table was historically. Your experiment hardware should have sufficient capacity that you don't have to reorganise it multiple times a day just to support multiple experiments.

You should be able to compile new shots easily at any time in the future, particularly if you want to see how a bugfix changed compiled instructions in runviewer - in which case you would want both a new shot and old shot loaded. A new shot can easily be compiled by loading the globals from the old shot file provided you don't also have to dig out the connection table. Personally I think there is a big difference between being able to achieve something using the graphical tools and having to extract data by hand from a HDF5 file.

Again, I get that achieving these goals isn't always possible in every lab. But from a documentation point of view, I think we should be showing best practices that follow the intended design - and as far as I can see (and remember from the many hours of discussion at Monash when we developed the system) that is not importing the connection table into your experiment logic.

changes in the master connection table would require explicit communication with other lab members (who are not present in the lab) so that they update their individual connection tables before they next run shots. Whereas changing a master connection table requires communication so that others know what happened, but no actual action by other lab users.

Yes, but isn't it better practice to be forced to see exactly what has been rewired in the lab to make sure there aren't any unintended consequences for your experiment logic?

I will push harder on recommending importing your connection table once setting device properties post-instantiation is more sensible. That's the main impediment I see to it being almost always the better approach.

I doubt this will change my mind. Fundamentally the BLACS connection table is supposed to include a definition of every device and channel in use on the apparatus. Experiment logic files should only include what is needed for that particular experiment. In some cases, this subset my be dynamically chosen at compile time based on globals (for example side vs. top imaging or not needing an entire Novatech DDS device because you are not using the second atomic species on a particular shot). I also fundamentally believe that being forced to update your connection table by hand is a rigorous check that ensures you comprehend that changes to the lab wiring.

@chrisjbillington
Copy link
Member

chrisjbillington commented Jun 21, 2020

Well, I won't die on this hill now or later (so I think @rpanderson you should just merge with or without the __main__ block however you prefer), but I think the disagreement comes down to how much weight to give to simplifying common use cases at the expense of less common ones.

The benefits make sense, but are uncommon to the point of being hypothetical, whereas the proposed code duplication will involve manually syncing files regularly. I think I actually set a device property on a per-shot basis once, which is why I consider that less hypothetical (and this is the entire intent of the distinction between device properties and connection table properties).

Code duplication is usually considered poor practice too, and whilst it's sometimes the right thing for small snippets, connection tables are a considerable amount of duplication. Importing a connection table is the simpler option, and having to update it manually, whilst on the one hand a manual check of understanding, is also itself error prone. Many faced with updating their connection table will simply do a copy-and-paste, a strong sign that it should have been an import, and skipping any benefit of carefully considering anyway.

There are many more reasons why old shots won't run (actual changes to the apparatus, globals being reorganised) than just the connection table, having to use version control to revert back to an old connection table, or extracting it from an old shot file is going to be the least of one's problems doing so. I see the connection table as almost just another module in labscriptlib. Just like a module encapsulating one stage BEC production, used by multiple experiment scripts, may change in a way that breaks old shots, so may the connection table. Running an old shot therefore involves either reverting the state of labscriptlib as a whole, or updating the experiment script to work with the new module. The connection table is just one more such module. In fact, even if you do have a distinct connection table, you might still put it in a separate module. What's the difference whether BLACS uses that module as well as its connection table? The fact that you have to restart BLACS when you change it? That seems preferable to me than having to keep two files in sync.

Regarding devices that are included in the shot file or not depending on globals, that strikes me as a contrived example. If both devices are regularly used, they should just both be in the connection table - why not? If one is not used for an extended period of time, it can be commented out of the connection table. What's the benefit of doing this with a global? What's the benefit of keeping connection table minimal at all?

My guiding philosophy is to make the most common workflows as simple as possible, and I don't mind making uncommon ones harder so long as they are still possible (that's why I refer to 'forensics' - the data on the connection table is there in the shot file if you really need it, but this is once in a blue moon instead of part of your normal workflow so it doesn't matter that it's inconvenient). If the uncommon use cases become more common, I would like to come up with other ways of making them easier that do not trade off against making the common use cases harder. But they have to be actually common, and I find our disagreements are often about hypothetical workflows that on order zero people are using. I think this is the root of our disagreement.

@philipstarkey
Copy link
Member

... I think @rpanderson you should just merge with or without the __main__ block however you prefer...

I think including the __name__ == '__main__' block is a good idea.

Code duplication is usually considered poor practice too, and whilst it's sometimes the right thing for small snippets, connection tables are a considerable amount of duplication.

I agree that code duplication is poor practice provided the code serves the same purpose. I disagree with the premise that the BLACS connection table should be considered identical to the experiment connection table. If that were the case, why do we check if the shot connection table is a subset of the BLACS connection table and not just do equality?

Of course for some experiments, it makes sense that the two connection table definitions be identical in every case. Other's it does not. For the purposes of documentation, it should be clear that they can, and in some cases (the prevalence of which we don't know) should, be different.

Regarding devices that are included in the shot file or not depending on globals, that strikes me as a contrived example. If both devices are regularly used, they should just both be in the connection table - why not? If one is not used for an extended period of time, it can be commented out of the connection table. What's the benefit of doing this with a global? What's the benefit of keeping connection table minimal at all?

This was actually the least contrived example in my previous post. It was the lived reality of the dual species lab at Monash for years. Specifically, we had camera systems that couldn't handle 0 exposures, so we would only include specific cameras if we were using them in a given shot. Our Novatech devices were split along atomic species lines. Excluding the Potassium Novatechs from Rubidium only experiments ensured we didn't need to synchronise globals (for those potassium DDS channels) between experiments just to keep the AOMs at the right power and/or frequency (important if an AOM is used in a laser lock and/or single pass configuration where pointing stability can be affected by temperature which is closely linked to RF power). Not using a device in a shot meant it kept all it's values from the last shot that did use it, avoiding discontinuities in output without having to synchronise a default state across experiment scripts. We did something similar for dipole trap AOMs.

My guiding philosophy is to make the most common workflows as simple as possible, and I don't mind making uncommon ones harder so long as they are still possible (that's why I refer to 'forensics' - the data on the connection table is there in the shot file if you really need it, but this is once in a blue moon instead of part of your normal workflow so it doesn't matter that it's inconvenient). If the uncommon use cases become more common, I would like to come up with other ways of making them easier that do not trade off against making the common use cases harder. But they have to be actually common, and I find our disagreements are often about hypothetical workflows that on order zero people are using. I think this is the root of our disagreement.

Sure. But given that the number of labs we've observed on a day-to-day basis is at least an order of magnitude less than the number of labs using the labscript suite, I don't think either of us knows what is common or uncommon. So maybe I've been focusing on the wrong arguments.

For documentation, I think we should be aiming for what best explains the fundamental principles of the labscript suite with the least opportunity for confusion. I believe it is important to make the distinction that the BLACS connection table is distinct from the experiment logic connection table, and that the latter only needs to be a subset of the former. I believe this is best achieved by not importing a common connection table in our example code.

@rpanderson rpanderson force-pushed the example_experiment branch from 4ed303c to 4c19de9 Compare June 22, 2020 07:45
@rpanderson
Copy link
Member Author

Thanks for fleshing out the pros/cons of importing connection tables here, @chrisjbillington @philipstarkey. I'll stick to what I signalled here and advertise the use of labscript_utils.import_or_reload in a tutorial, which can include a distilled version of these pros/cons.

@chrisjbillington chrisjbillington merged commit 1189334 into labscript-suite:master Jun 24, 2020
@rpanderson rpanderson deleted the example_experiment branch June 25, 2020 00:57
philipstarkey added a commit that referenced this pull request Jun 25, 2020
commit d8b8343
Merge: 9bc7a39 bfa6932
Author: Chris Billington <[email protected]>
Date:   Wed Jun 24 20:48:39 2020 -0400

    Merge pull request #59 from philipstarkey/fix-h5py-file-mode

    Addresses #47 for this module

commit 9bc7a39
Merge: 1189334 66d0be4
Author: Chris Billington <[email protected]>
Date:   Wed Jun 24 20:30:42 2020 -0400

    Merge pull request #55 from rpanderson/example_IMAQdx_remote

    Example labscript and analysis scripts for IMAQdxCamera and RemoteBLACS

commit bfa6932
Author: philipstarkey <[email protected]>
Date:   Thu Jun 25 10:25:49 2020 +1000

    Addresses #47 for this module

commit 1189334
Merge: 5025b1b 0b0d07a
Author: Chris Billington <[email protected]>
Date:   Wed Jun 24 19:35:15 2020 -0400

    Merge pull request #54 from rpanderson/example_experiment

    Minimal working connection table and example experiment script

commit 66d0be4
Author: Russell Anderson <[email protected]>
Date:   Mon Jun 15 13:37:23 2020 +1000

    lyse analysis script demonstrating some Run methods for example_IMAQdx_remote

commit a48a9c0
Author: Russell Anderson <[email protected]>
Date:   Mon Jun 15 13:15:48 2020 +1000

    Example connection table and labscript experiment script for RemoteBLACS and IMAQdxCamera

commit 5025b1b
Merge: f77525f ed76901
Author: Chris Billington <[email protected]>
Date:   Wed Jun 24 02:16:29 2020 -0400

    Merge pull request #58 from philipstarkey/philipstarkey/issue43

    Fixes .pth file has no effect in editable install

commit ed76901
Author: chrisjbillington <[email protected]>
Date:   Wed Jun 24 01:23:44 2020 -0400

    Move copying of .pth file into setup.py

    Use a custom develop command to copy the .pth file into site-packages
    when `python setup.py develop` is run - this is what pip does under the
    hood for `pip install -e`.

    No longer need desktop-app to locate site-packages, as the setuptools
    command class knows it as `self.install_dir`.

    The file is not removed by `pip uninstall`, but due to the `try:
    except:`s in the `.pth` file, leaving it behind is harmless.

    It is removed if the user runs whatever `setup.py` command undoies

commit 894e245
Author: chrisjbillington <[email protected]>
Date:   Wed Jun 24 00:04:13 2020 -0400

    Require desktop-app 0.2.6

    Which renames `_get_install_directory` to not have an underscore,
    and which fixes a bug preventing docs from building on RTD.

commit 271439e
Author: philipstarkey <[email protected]>
Date:   Wed Jun 24 12:53:49 2020 +1000

    Removed support for custom development directories from path

commit dd098a5
Author: philipstarkey <[email protected]>
Date:   Wed Jun 24 12:38:19 2020 +1000

    Fixes .pth file has no effect in editable install
    Fixes #43

    The .pth file is now copied during `labscript-profile-create` if it does not exist in the site-packages dir (where the .egg-info files exist) and if it the .pth file exists in the package structure (aka labscript-utils it is an editable install)

    The labscript-suite.pth file has been updated to not raise an exception if labscript-suite has been uninstalled (so we do not have to worry about cleanup).

    Also updated the `labscript_profile.add_userlib_and_pythonlib` function to use the `site` library for adding userlib and pythonlib to the system path.

commit 0b0d07a
Author: Russell Anderson <[email protected]>
Date:   Mon Jun 22 17:48:00 2020 +1000

    start()/stop() in __main__ block so connection table can be imported

commit 4c19de9
Author: Russell Anderson <[email protected]>
Date:   Mon Jun 15 13:12:52 2020 +1000

    Add AnalogOut with ramp example, rename DigitalOut connection

commit 9b64a0d
Author: Russell Anderson <[email protected]>
Date:   Wed Jun 10 23:09:03 2020 +1000

    Minimal example connection table and labscript using dummy devices

commit f77525f
Author: Phil Starkey <[email protected]>
Date:   Sat Jun 20 18:09:53 2020 +1000

    Removed unnecessary fixed title from main TOC

    This was erroneously added in #57

commit 5e02b01
Merge: 3f78207 8b19766
Author: Phil Starkey <[email protected]>
Date:   Sat Jun 20 18:07:04 2020 +1000

    Merge pull request #57 from philipstarkey/master

    Changed URL for API reference to be shorter
    Fixed bug with readthedocs auto PR build

commit 8b19766
Author: philipstarkey <[email protected]>
Date:   Sat Jun 20 12:02:51 2020 +1000

    Updated method call for stylesheet to remove deprecated warning in build

commit 56fdd63
Author: philipstarkey <[email protected]>
Date:   Sat Jun 20 11:46:33 2020 +1000

    Fixed sphinx conf to detect PR builds and link to 'latest' instead of stable

commit 9889d19
Author: philipstarkey <[email protected]>
Date:   Sat Jun 20 10:44:04 2020 +1000

    Changed URL for API reference to be shorter

commit 3f78207
Merge: bbd928a bf9d10e
Author: Phil Starkey <[email protected]>
Date:   Fri Jun 19 18:58:37 2020 +1000

    Merge pull request #56 from philipstarkey/master

    Added links to other component docs

commit bf9d10e
Author: philipstarkey <[email protected]>
Date:   Fri Jun 19 16:06:09 2020 +1000

    Addressing latest review comments

commit e75b6bc
Author: philipstarkey <[email protected]>
Date:   Fri Jun 19 11:23:49 2020 +1000

    Updated docs to use jinja template for component doc links

commit 9c61238
Author: philipstarkey <[email protected]>
Date:   Thu Jun 18 17:14:57 2020 +1000

    Removed autogenerated rst file from version control

commit 2303185
Author: philipstarkey <[email protected]>
Date:   Thu Jun 18 16:43:29 2020 +1000

    Updated .gitignore with latest GitHub defaults

commit 87b757e
Author: philipstarkey <[email protected]>
Date:   Thu Jun 18 16:39:52 2020 +1000

    Relocated custom sphinx gitignore rules

commit 75813a5
Author: philipstarkey <[email protected]>
Date:   Thu Jun 18 16:31:01 2020 +1000

    Updated PR as per review in labscript-suite/labscript-suite#48

commit bbd928a
Author: Russell Anderson <[email protected]>
Date:   Wed Jun 17 11:39:17 2020 +1000

    Populated README.md with styling, iconogrpahy, prose, and badges

commit 573c521
Author: philipstarkey <[email protected]>
Date:   Tue Jun 16 18:10:25 2020 +1000

    Added links to other component docs
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

Successfully merging this pull request may close these issues.

3 participants