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 52 forks source link

Make example labscript functional and format using black #69

Closed rpanderson closed 4 years ago

rpanderson commented 4 years ago

This PR fixes example.py which presently does not compile without warnings/errors, and omits the wildcard import (from labscript import *).

An alternative would be to remove this example now that we have established a convention of writing and distributing examples in the default_profile/userlib/labscriptlib/example_apparatus folder of labscript-utils.

chrisjbillington commented 4 years ago

Just out of interest, why did it not compile? Skimming it I don't see anything that jumps out as broken.

Edit: Oh I guess it it was the import __init__, which is not needed and breaks since the restructuring

rpanderson commented 4 years ago
import __init__

yields:

Traceback (most recent call last):
  File "C:\Users\rpanderson\labscript-dev\labscript\example.py", line 14, in <module>
    import __init__ # only have to do this because we're inside the labscript directory
RuntimeError: Double import! The same file has been imported under two different names, resulting in two copies of the module. This is almost certainly a mistake. If you are running a script from within a package and want to import another submodule of that package, import it by its full path: 'import module.submodule' instead of just 'import submodule.'

Path imported: C:\Users\rpanderson\labscript-dev\runmanager\runmanager\__init__.py
NI_PCIe_6363(name='ni_card_0', parent_device=pulseblaster_0_clockline_fast, clock_terminal='ni_pcie_6363_0/PFI0', MAX_name='ni_pcie_6363_0', acquisition_rate=100e3)

yields:

C:\Users\rpanderson\labscript-dev\labscript\example.py:27: UserWarning: Importing NI_PCIe_6363 from labscript_devices.NI_PCIe_6363 is deprecated, please instead import it from labscript_devices.NI_DAQmx.labscript_devices. Importing anyway for backward compatibility, but this may cause some unexpected behaviour.

Ditto for NI_PCI_6733.

Remaining issue resolved by this PR:

WARNING: State of analog output analog0 at t=1.00000035s has already been set to 2. Overwriting to linear ramp. (note: all values in base units where relevant)
Traceback (most recent call last):
  File "C:\Users\rpanderson\labscript-dev\labscript\example.py", line 182, in <module>
    stop(t)
labscript.labscript.LabscriptError: Commands have been issued to devices attached to pulseblaster_0_pseudoclock at t= 1.0 s and 1.0000003466666667 s. One or more connected devices on ClockLine pulseblaster_0_clockline_fast cannot support update delays shorter than 3.5e-07 sec.
Compilation aborted.
philipstarkey commented 4 years ago

I'm not convinced a black formatted connection table is easier to parse than a series of very long lines. Maybe it would be OK if each argument was turned into a keyword argument so it was obvious what it was? Even then I'm not sure

chrisjbillington commented 4 years ago

I find it easier to parse, regardless of whether the args have names or not (though once there are more than a few, keyword argument names are basically mandatory for code to be readable at all). It's definitely something you get used to, but once you're used to it newlines are stronger delimiters than commas and spaces - so one can see at a glance how many args there are and is less likely to confuse e.g. two short args for one long one.

At the end of the day though this is less important to me than the fact that it fits in a text editor snapped to half the screen. Having to scroll or maximise to read, and having scroll to view diffs (which are also much improved when each arg is on its own line) is the dealbreaker for me.

Also it's the standard. I would not have made all the choices black did (spaces around the slice operator x[i : i + 2], what?), but I'll accept them to never have to not have to think about formatting python code anymore.

rpanderson commented 4 years ago

Named all args in connection table, device methods, and other labscript primitives.

philipstarkey commented 4 years ago

Looks better!

It is a bit odd having the example in here now it's a proper package. Maybe we should relocate it to labscript-utils? The existing example is very basic. It's good to have a basic example, but perhaps this could serve as a complex example in the default profile?

rpanderson commented 4 years ago

Yep, I edited the PR description earlier to countenance this alternative. I'd be happy to migrate or rewrite this example in labscript-utils, perhaps separately to (and after) merging this PR.

rpanderson commented 4 years ago

It is a bit odd having the example in here now it's a proper package.

Were it not for the use of labscript-utils, it's not entirely unconventional. Examples would otherwise be useful here for testing and for sourcing documentation from. But I prefer the convention of using the default profile in labscript-utils, as this means they get distributed.

philipstarkey commented 4 years ago

They only get distributed when people first create the profile though. Subsequent updates we make will be buried in the package just like they are here.

rpanderson commented 4 years ago

Yes; these would benefit from a labscript-profile-update function to complement labscript-profile-create.

philipstarkey commented 4 years ago

Well I'm happy to merge as-is then