search

nanophotonics / nplab

Core functions and instrument scripts for the Nanophotonics lab experimental scripts
GNU General Public License v3.0
38 stars 15 forks source link

First pass at #124 #126

Open YagoDel opened 3 years ago

YagoDel commented 3 years ago

It solves my issue with the Andor (stops setting values in the Initializemethod), 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

eoinell commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

YagoDel commented 3 years ago

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

YagoDel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 @eoinellhttps://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 GitHubhttps://github.com/nanophotonics/nplab/pull/126#issuecomment-786520865, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKORCB54FN2SOWENPSLQ6UTTA5R5JANCNFSM4SSWD4JA.

YagoDel commented 3 years ago

Thanks! I’ll keep an eye out for it

On Fri, 26 Feb 2021 at 18:33, Eoin Elliott notifications@github.com wrote:

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< https://github.com/nanophotonics/nplab/pull/126#issuecomment-786520865>, 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 https://github.com/nanophotonics/nplab/pull/126#issuecomment-786527429, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFEQCM3R5BXCQPKZJXASOLTA5TIHANCNFSM4SSWD4JA .

YagoDel commented 3 years ago

Do the above comments mean you have tested this branch?

eoinell commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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\instrumentinit.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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 GitHubhttps://github.com/nanophotonics/nplab/pull/126#issuecomment-804005941, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKORCB7J25FQ6JAGP4MDKXTTE4V6BANCNFSM4SSWD4JA.

YagoDel commented 3 years ago

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 commented 3 years ago

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 GitHubhttps://github.com/nanophotonics/nplab/pull/126#issuecomment-804027017, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKORCB4I5HOY5NPVPCECVSTTE42RZANCNFSM4SSWD4JA.

YagoDel commented 3 years ago

Ok, so three things:

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.