search

labscript-suite-temp-2 / labscript_utils

Shared modules used by the labscript suite. Includes a graphical exception handler, debug tools, configuration management, cross platform filepath conversions, unit conversions and custom GUI widgets.
Other
0 stars 0 forks source link

Unitconversions module does not belong in labscript_utils #3

Open philipstarkey opened 10 years ago

philipstarkey commented 10 years ago

Original report (archived issue) by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

The unitconversions module does no belong in labscript_utils. Unit conversion classses update and require committing often, and should not be in sync with the rest of labscript_utils, which provides application libraries that change less often. They should be considered 'user space' code, like labscript_devices is.

unitconversions should be its own module at the toplevel, or else part of labscript_devices. My preference leans toward the former, especially seeing that it is not coupled with other devices, and that we now have an automated installer making the minimisation of the number of packages less important (though I would not propose splitting off the rest of labscript_utils, with possible exceptions due to some of them having general interest outside of labscript, like excepthook and h5_lock, which may be released separately one day).

For backward compatability, labscript_utils.unitconversions should continue to import the separate module to provide seamless functionality, but should print a deprecation warning. This code should be tagged with a comment # DEPRECATED or similar, so it can be seen and removed at a major version bump. (this approach should be followed for backward compatibility issues in general)

philipstarkey commented 10 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

philipstarkey commented 10 years ago

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

I have a different idea for this that might be better.

You keep UnitConversionBase.py in labscript utils (move it to top level) as that should not change very often. You make the user specify the python module name in lab config (or multiple module names) which contain the unitconversion classes. The name specified must be importable with import from a Python terminal.

We can autocreate a module in the Pythonlib folder new users, but this gives "power users" the flexibility to do what they want with the location of the unitconversions.

thoughts?

philipstarkey commented 10 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

I like the base class being in labscript_utils, as yes it won't change often. This is similar to classes in labscript_devices being subclasses of labscript.Device.

But I would otherwise prefer to keep a default directory full of unit conversions, since some are useful to particular labscript device classes (like the novatech ones), and so should be in every install, and there is generally the desire to encourage people to commit back. My suggestion to separate 'user devices' from 'core devices' and allow users to put new device classes in arbitrary places was decided against in our meeting the other day for this reason (and I agree, my idea was stupid :p). So I would argue that by the same logic all unit conversions should be together and in a default location, easily committed back.

We are actually using a few of the conversion classes you wrote already!

philipstarkey commented 10 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

Hey Phil, another issue got me thinking, and I think we can have the best of both worlds, with unitconversions not in labscript_utils, but able to be anywhere in the Python search path, and also not have the user have to specify anything in labconfig. See here (#4) for the proposal. And not even any magic! Everything totally explicit and with no extra inconvenience to the user.

philipstarkey commented 10 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

Oh, but I think there should still be a unit conversions module that lives in a default place, to provide people with the existing classes and encourage them to contribute back.

philipstarkey commented 10 years ago

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

That sounds good. I am still a bit reluctant to make another repo just for uni conversions, but don't have a better idea at the moment.

philipstarkey commented 10 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

How about we put it inside labscript_devices? I think I would prefer that actually. It's conversions for the devices after all :p

philipstarkey commented 10 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

I mean put the unitconversions folder within labscript_devices - so the devices and unit conversion files would not be freely mingling with each other, but you wouldn't be allowed to call a labscript device "unitconversions", not that you would want to.