First pass at #124 by YagoDel · Pull Request #126 · nanophotonics/nplab

YagoDel · 2020-10-16T01:09:43Z

It solves my issue with the Andor (stops setting values in the Initialize method), but haven't tested it with other instruments and don't know if it's useful for anything else, so I'm not closing the #124 yet

…perty names

eoinell · 2020-10-18T12:12:22Z

yaml and pymsgbox aren't in the standard library, right? Would it be ok to use either stdlib file formats such as json or pickle, or our favourite .h5? And could the messagebox be done in plain pyqt? Just worried about introducing more dependencies, particularly for guis. Even tkinter is stdlib and has some simple msgbox options, even though it would be weird to mix pyqt and tkinter like that.

Since save_config is called through the console, can we just use a good old fashioned input()?

Really nice idea though, there are plenty of instruments that would benefit from this, I usually set default attributes in a meta 'lab' instrument which holds all the cooperative methods, and initialization functions for each instrument. This does extra work though for instruments with default parameters already.

YagoDel · 2020-10-18T13:21:26Z

I'm with you on the GUI dependencies. I'll see if I can figure out some simple PyQt ones (last time I tried I found it annoying to synchronise within the QtApp)

Really unsure what you mean with the input() comment

Pickle and HDF5 aren't human-readable, so I'm not a huge fan for config files. In terms of YAML vs JSON, I might have a quick go at how JSON handles general Python types. I think YAML more naturally handles things. For example, it uses indentation for hierarchy, which is neat for Python users (see the YAML I pushed to this branch). But I haven't played with JSON much and it might be good too.

rwb27 · 2020-10-19T23:21:19Z

I've found JSON and YAML similar for basic types - YAML is slightly more pythonic in structure but both work fine. I don't think JSON has out of the box support for serialising python types, but TBH that's not so human readable in YAML either, so I tend to wrap things in conversion functions to make sure i only load/save simple types.

…attribute because it was a hack

YagoDel · 2020-10-23T02:01:53Z

Got it working with JSON and PyQt. Not really sure there's much more to do in this PR.

Maybe integrating with the previous config code which seems to be used to save spectrometer/CCD background/references (and a couple of more things), but I'm gonna need someone to test it

…issue124

… new code with JSON. First step in moving away from named configuration settings

…breeze

…y files

YagoDel · 2020-10-28T05:01:25Z

Integrated with previous configuration code. I've tested it with my SeaBreeze spectrometer, and it seems the functionality is all good to go, but someone else testing it would be ideal

Added the possibility of saving all of an instruments parameters.
For super-basic stuff, it's possible to simply call
instrument.config_file = instrument.config to save and instrument.config = instrument.config_file to load

YagoDel · 2021-02-26T09:22:14Z

I am starting to need features I've developed in different branches, so I'm going to merge this next week

It would be nice to check that it doesn't break anything beforehand (I'm not sure who is currently actively working on nplab @eoinell), otherwise I'll just be active to help if it does break anything after

eoinell · 2021-02-26T09:33:39Z

I'll have a quick spin next time I'm in the lab!

…

________________________________ From: YagoDel <notifications@github.com> Sent: Friday, February 26, 2021 9:22:28 AM To: nanophotonics/nplab <nplab@noreply.github.com> Cc: Eoin Elliott <ee306@cam.ac.uk>; Mention <mention@noreply.github.com> Subject: Re: [nanophotonics/nplab] First pass at #124 (#126) I am starting to need features I've developed in different branches, so I'm going to merge this next week It would be nice to check that it doesn't break anything beforehand (I'm not sure who is currently actively working on nplab @eoinell<https://github.com/eoinell>), otherwise I'll just be active to help if it does break anything after — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#126 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKORCB54FN2SOWENPSLQ6UTTA5R5JANCNFSM4SSWD4JA>.

YagoDel · 2021-02-26T10:37:48Z

Thanks! I’ll keep an eye out for it

…

On Fri, 26 Feb 2021 at 18:33, Eoin Elliott ***@***.***> wrote: I'll have a quick spin next time I'm in the lab! ________________________________ From: YagoDel ***@***.***> Sent: Friday, February 26, 2021 9:22:28 AM To: nanophotonics/nplab ***@***.***> Cc: Eoin Elliott ***@***.***>; Mention ***@***.***> Subject: Re: [nanophotonics/nplab] First pass at #124 (#126) I am starting to need features I've developed in different branches, so I'm going to merge this next week It would be nice to check that it doesn't break anything beforehand (I'm not sure who is currently actively working on nplab @eoinell< https://github.com/eoinell>), otherwise I'll just be active to help if it does break anything after — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub< #126 (comment)>, or unsubscribe< https://github.com/notifications/unsubscribe-auth/AKORCB54FN2SOWENPSLQ6UTTA5R5JANCNFSM4SSWD4JA >. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADFEQCM3R5BXCQPKZJXASOLTA5TIHANCNFSM4SSWD4JA> .

eoinell · 2021-03-02T11:00:52Z

nplab/instrument/__init__.py

        """Create an instrument object."""
        super(Instrument, self).__init__()
-        Instrument.instances_set().add(self) #keep track of instances (should this be in __new__?)
+        Instrument.instances_set().add(self)  # keep track of instances (should this be in __new__?)


I don't think using new is necessary

eoinell · 2021-03-02T11:01:24Z

nplab/instrument/__init__.py

        """
        instances = cls.get_instances()
-        if len(instances)>0:
+        if len(instances) > 0:


if instances:

eoinell · 2021-03-02T11:06:24Z

nplab/instrument/camera/Andor/default_config.json

@@ -0,0 +1 @@
+{"ReadMode": 4, "AcquisitionMode": 1, "TriggerMode": 0, "Exposure": 0.01, "Shutter": [1, 0, 1, 1], "SetTemperature": -90, "CoolerMode": 0, "FanMode": 0, "cooler": 0}


can exposure be 1s? most common value in our labs for Si referencing.

Sure, I'm not using the default_config anyway. In that case, do you know what your shutter speeds are?

I don't know, so probably setting it to (1,0,0,0) is most sensible..

I think that'll fail for most shutters. Which could be good, since it'll force people to actively set their own configurations), but could also be a pain in the ass

Ok, nvm, it seems that's what master does now anyway, so yeah Shutter=(1, 0, 0, 0) it is

eoinell · 2021-03-02T11:08:23Z

nplab/instrument/__init__.py

 """

 from builtins import str
-from nplab.utils.thread_utils import locked_action_decorator, background_action_decorator


locked_action and background_action are nicer aliases IMO

Could agree, not an issue for this branch :P (it's been like that since the start of nplab)

YagoDel · 2021-03-04T05:28:59Z

Do the above comments mean you have tested this branch?

eoinell · 2021-03-22T10:59:01Z

I just mean use the console for input instead of a pyqt popup if it's easier - but you have this working now so nvm :)

Just tried it now (sorry for the delay, tbh I prefer to let the rest of nplab do a bit of involuntary testing once i'm pretty sure it'll work)
`
File "C:\Users\hera\Documents\GitHub\nplab\nplab\instrument_init_.py", line 315, in load_config
with open(filename, 'r') as config_file:

FileNotFoundError: [Errno 2] No such file or directory: 'C:\Users\hera\Documents\GitHub\nplab\nplab\instrument\spectrometer\default_config.json'`

This is for the andor - I guess it needs to handle the case where there isn't a default_config file?

Sorry this is a bit late in the game, but I can't help but feel that this would be better placed in the Instrument, where it would check for the existence of a default config file, and if it finds one attempt to set all the parameters in it. Otherwise it just acts as normal.

eoinell · 2021-03-22T11:33:10Z

If these config files can't be implemented without having to change the respective instruments' code then they'll just become one more thing in the instrument base class that nobody knows how or why to use. Ideally calling instr.save_config() should be all that's needed to get going. This save call should also try to set all parameters, and remove any that fail. The properties to be set could be integrated with the metadata_property_names system possibly, but this is probably unnecessary.

YagoDel · 2021-03-22T11:51:31Z

This is useful stuff, thanks!

Sorry this is a bit late in the game, but I can't help but feel that this would be better placed in the Instrument, where it would check for the existence of a default config file, and if it finds one attempt to set all the parameters in it. Otherwise it just acts as normal.

So the config stuff all happesn in the Instrument base (same as it was happening before, but before it was only using HDF5, and I don't think that many instruments were using it)

Just tried it now (sorry for the delay, tbh I prefer to let the rest of nplab do a bit of involuntary testing once i'm pretty sure it'll work)
`
File "C:\Users\hera\Documents\GitHub\nplab\nplab\instrument__init__.py", line 315, in load_config
with open(filename, 'r') as config_file:

FileNotFoundError: [Errno 2] No such file or directory: 'C:\Users\hera\Documents\GitHub\nplab\nplab\instrument\spectrometer\default_config.json'`

This is for the andor - I guess it needs to handle the case where there isn't a default_config file?

I'll have a look to fix the issue of it failing (it should be simple enough). You got that error just from trying to create an Andor instance? It feels like a Trandor or Kandor instance

YagoDel · 2021-03-22T11:56:00Z

I very much appreciate the want for a tool that can be used by all users. I'm not sure I understand how an instrument instance can know what it's own "parameters" are though.

So specifically, when you say "set all parameters, and remove any that fails", I'm not sure how to do it, other than manually giving it a list of names (which in this case was config_property_names, in parallel with metadata_property_names, but I'm happy to look into either merging them into a single one, or having a hierarchy)

eoinell · 2021-03-22T12:07:59Z

Hmm, yeah.. maybe a tuple of all settable parameter names similar to metadata_property_names but this requires some extra code. In camera.thorlabs.kiralux I implemented something where it would just add everything that has a getter and setter method, and remove any that fail but this may be a bit too general. You could explicitly filter properties that come from instrument..
Basically I'm pretty sure this will work but you may end up with too much stuff (like live_view in Camera for example) being in the config file.

A shamdor instance, yep. I guess there's a bit of a distinction between settings parameters (JSON is perfect here, and this should happen on every instantiation) and configs like background/reference spectra which are arguably better rendered with the h5 renderer and should be optionally loaded.

eoinell · 2021-03-22T12:25:37Z

This config_property_names system probably works well though – sorry I forgot what was in this commit exactly. From: YagoDel ***@***.*** Sent: Monday 22 March 2021 11:56 To: nanophotonics/nplab ***@***.***> Cc: Eoin Elliott ***@***.***>; Comment ***@***.***> Subject: Re: [nanophotonics/nplab] First pass at #124 (#126) I very much appreciate the want for a tool that can be used by all users. I'm not sure I understand how an instrument instance can know what it's own "parameters" are though. So specifically, when you say "set all parameters, and remove any that fails", I'm not sure how to do it, other than manually giving it a list of names (which in this case was config_property_names, in parallel with metadata_property_names, but I'm happy to look into either merging them into a single one, or having a hierarchy) — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#126 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKORCB7J25FQ6JAGP4MDKXTTE4V6BANCNFSM4SSWD4JA>.

YagoDel · 2021-03-22T12:35:23Z

Oh, I'll have a look at the kiralux thing, but you're probably right we'd end up with tons of stuff.

I guess I hadn't realised people were actually using the HDF5 config code (that is currently in the Instrument base) for spectrometer, but I had tried to keep it compatible. I guess it breaks down if it tries to do both things (read configuration parameters from a JSON, and config for a spectrometer)

Do you know if the current config code is used for anything else?

eoinell · 2021-03-22T12:55:34Z

Camera with location uses it for the pixel_to_sample_displacement

…

________________________________ From: YagoDel ***@***.***> Sent: Monday, March 22, 2021 12:35:40 PM To: nanophotonics/nplab ***@***.***> Cc: Eoin Elliott ***@***.***>; Comment ***@***.***> Subject: Re: [nanophotonics/nplab] First pass at #124 (#126) Oh, I'll have a look at the kiralux thing, but you're probably right we'd end up with tons of stuff. I guess I hadn't realised people were actually using the HDF5 config code (that is currently in the Instrument base) for spectrometer, but I had tried to keep it compatible. I guess it breaks down if it tries to do both things (read configuration parameters from a JSON, and config for a spectrometer) Do you know if the current config code is used for anything else? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#126 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKORCB4I5HOY5NPVPCECVSTTE42RZANCNFSM4SSWD4JA>.

YagoDel · 2021-03-28T09:44:15Z

Ok, so three things:

Can't find the <camera.thorlabs.kiralux> you mentioned
The reason opening a Shamdor failed for you is that it doesn't know where to look for the default parameters. The way I wrote the code is that if the specified path isn't absolute, it tries to make an absolute path from the location of the class. Since the Andor and the Shamdor are in different directories, it fails. I've corrected it in the latest commit, by forcing the Andor to always look in the Andor directory if a config_file path is not given on instantiation. So it's fixed, have a test again.

And finally, following what you said:

I guess there's a bit of a distinction between settings parameters (JSON is perfect here, and this should happen on every instantiation) and configs like background/reference spectra which are arguably better rendered with the h5 renderer and should be optionally loaded.

I foresee one possible issue. I tried to make the config code backwards compatible, so that instruments could either save to HDF5 or JSON and the rest of nplab would continue working as it was. But if we create an instrument that is a subclass of an instrument that does config in HDF5 and an instrument that does a config in JSON (for example a camera_with_location and and Andor), then I'm fairly certain it'll fail.The simplest fix is to just make camera_with_location save that value to a JSON instead.

YagoDel added 4 commits October 15, 2020 19:36

Save instrument configuration to YAML following to a set of given pro…

a2feb24

…perty names

Removing hardcoded parameters and moving them to configuration files

f4ed033

Default Andor parameters

0b2a15c

Corrected defaults

5caed41

YagoDel requested a review from eoinell October 16, 2020 01:11

YagoDel added 4 commits October 22, 2020 16:59

Utility PyQt message boxes following pymsgbox

86181db

Saving config to JSON. Using PyQt for warning message boxes

fd44e10

Default Andor JSON

7911fc9

Debugged some of the amplifier and channel handling. Removed channel …

b54b267

…attribute because it was a hack

YagoDel added 9 commits October 28, 2020 09:42

Merge branch 'master' of https://github.com/nanophotonics/nplab into …

be95189

…issue124

CameraHandle debug

ab56ef0

First pass at integrating old configuration code with HDF5 files with…

e5fe15e

… new code with JSON. First step in moving away from named configuration settings

Debug

31ef641

Might need reverting. Removing old config code

7b50dc2

config_file for ease of use

b80cb99

Might need reverting. Updated spectrometer config and tested with Sea…

1b3fcc1

…breeze

Debugged update_config. No prompt if file exists already. Handle empt…

fa7c63e

…y files

Cleanup

b894eff

YagoDel and others added 3 commits October 28, 2020 14:12

Merge branch 'master' into issue124

e9e7d6d

Andor configuration loading using properties

54cbfdb

Merge from master

cdfc880

eoinell reviewed Mar 2, 2021

View reviewed changes

nplab/instrument/__init__.py

"""

instances = cls.get_instances()

if len(instances)>0:

if len(instances) > 0:

Copy link

Contributor

eoinell Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if instances:

eoinell reviewed Mar 2, 2021

View reviewed changes

YagoDel added 2 commits March 28, 2021 18:31

Default config always points to the correct location

4ca63bb

Bug

ad1fde5

		@@ -0,0 +1 @@
		{"ReadMode": 4, "AcquisitionMode": 1, "TriggerMode": 0, "Exposure": 0.01, "Shutter": [1, 0, 1, 1], "SetTemperature": -90, "CoolerMode": 0, "FanMode": 0, "cooler": 0} No newline at end of file

Conversation

YagoDel commented Oct 16, 2020

Uh oh!

eoinell commented Oct 18, 2020

Uh oh!

YagoDel commented Oct 18, 2020

Uh oh!

rwb27 commented Oct 19, 2020

Uh oh!

YagoDel commented Oct 23, 2020

Uh oh!

YagoDel commented Oct 28, 2020

Uh oh!

YagoDel commented Feb 26, 2021

Uh oh!

eoinell commented Feb 26, 2021 via email

Uh oh!

YagoDel commented Feb 26, 2021 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YagoDel commented Mar 4, 2021

Uh oh!

eoinell commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eoinell commented Mar 22, 2021

Uh oh!

YagoDel commented Mar 22, 2021

Uh oh!

YagoDel commented Mar 22, 2021

Uh oh!

eoinell commented Mar 22, 2021

Uh oh!

eoinell commented Mar 22, 2021 via email

Uh oh!

YagoDel commented Mar 22, 2021

Uh oh!

eoinell commented Mar 22, 2021 via email

Uh oh!

YagoDel commented Mar 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eoinell commented Mar 22, 2021 •

edited

Loading

YagoDel commented Mar 28, 2021 •

edited

Loading