remove settings from base classes

[carandraug]

This has been discussed a few times but I can't find a specific issue about it (I've found comments of it on #92 and #161) so I'm opening one now. I believe we always agreed on this and this was just not done yet because it's not a priority. Here is it now for future records.

Some ABCs add settings, e.g., Camera has transform, roi, and readout mode and FilterWheel has position. There's two reasons why we don't want this: 1) if it is defined on the ABC then it is useful to all concrete implementations at which point we should have a specialised method for it; and 2) settings should be for device specific parameters and we run the risk of clashing with their names (see #92 and #161). This means that some settings will replicate some ABC methods but that's ok. The settings are the changes at the level of the vendor library/interface while the ABC methods just need to be more careful to not break. This is referenced on the documentation:

Settings often overlap with the defined interface. For example, PVCamera instances have the binning property as defined on the Camera ABC, but if supported by the hardware they will also have the "BINNING_PAR" and "BINNING_SER" settings which effectively do the same.

Over time, we have removed most of those settings. I've recently also removed "positions" from FilterWheel (d5e6c39dc9f2), and "readout mode" from Camera (953d977df9ae).

The only remaining settings I see are "roi" and "transform" on Camera. I would like to remove them and have this sorted once and for all. Questions about them:

roi setting

Camera has set_roi and get_roi methods but Cockpit does not use them. Cockpit users wanting to select a ROI need to go through the camera settings menu and type in the ROI. This is terrible UI but that's what Cockpit has. @dstoychev mentioned having a patch that adds the option to draw a ROI on the camera view. This would be nice and then we could remove the roi setting.

transform setting (removed with ee7842a5b4b9)

Camera has a set_transform method (but no get_transform). Cockpit reads transform values for both objectives and cameras in depot.conf and passes them to Camera.set_transform (see both being used in https://github.com/MicronOxford/configs/blob/6bd326a93bc75b6b1f53a2fab43a1d0597b9d93d/DeepSIM/depot.conf). While it is possible to pass a transform to the camera in the depot.conf through settings, that's an advanced and undocumented feature and I don't think anyone is using it (and if they are, they should be using the transform key instead of settings). So I'm thinking it's safe to just remove this.

[carandraug]

I've now pushed ee7842a5b4b9 which removes the transform setting.

I've looked a bit deeper and found that for cameras we have readout_transform (transform after we read the image because different readout modes may return images in different orientations), client_transform (any transformation user wants to do), and transform (which is the merge of the two). Only PVCamera seems ot make use of all of this. I think there was also an issue on the setting because setting it would affect the client_transform but reading it would return the merged transform. Anyway, its now removed.

---

roi setting still needs to be removed.

[iandobbie]

We need to think very carefully before significantly changing the camera transform processing. The settings are being used on system in Oxford as this is the only current way to set the transform at cockpit startup. Additionally, the transform should happen on the microscope side rather than cockpit as multiple cameras can then be distributed over a number of computers to increase image throughput.

Sent from my iPhone

On Jan 6, 2023, at 9:09 AM, David Miguel Susano Pinto @.***> wrote:



I've now pushed ee7842ahttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpython-microscope%2Fmicroscope%2Fcommit%2Fee7842a5b4b96b73e3603946eed48874e1640a70&data=05%7C01%7Cidobbie1%40jh.edu%7C7f4f88f581d5471b790308daefefa026%7C9fa4f438b1e6473b803f86f8aedf0dec%7C0%7C0%7C638086109713003730%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=4tGatro1%2F1FQ43Oj9%2BWjvzlv5lMOF%2Fr57hy6KsmhSGc%3D&reserved=0 which removes the transform setting.

I've looked a bit deeper and found that for cameras we have readout_transform (transform after we read the image because different readout modes may return images in different orientations), client_transform (any transformation user wants to do), and transform (which is the merge of the two). Only PVCamera seems ot make use of all of this. I think there was also an issue on the setting because setting it would affect the client_transform but reading it would return the merged transform. Anyway, its now removed.

---

roi setting still needs to be removed.

— Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpython-microscope%2Fmicroscope%2Fissues%2F260%23issuecomment-1373692547&data=05%7C01%7Cidobbie1%40jh.edu%7C7f4f88f581d5471b790308daefefa026%7C9fa4f438b1e6473b803f86f8aedf0dec%7C0%7C0%7C638086109713003730%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=SNCBGye%2FeU4sXJyT3XjBVeoIXeBmNco88EVmHENNKog%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABJTBBNDX4ZWNBCZGXW3NA3WRARRBANCNFSM6AAAAAATTDGINM&data=05%7C01%7Cidobbie1%40jh.edu%7C7f4f88f581d5471b790308daefefa026%7C9fa4f438b1e6473b803f86f8aedf0dec%7C0%7C0%7C638086109713003730%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uEjRIaY804Uw8bB%2F6gILRmggyNYjzlBUGa9jPkzwrI0%3D&reserved=0. You are receiving this because you are subscribed to this thread.Message ID: @.***>

[iandobbie]

We need to test this. Both DeepSIM and CryoSIM use this setting in the depot.conf to set the camera orientations on startup. This is essential functionality as the two cameras have different orientations, as they are separted by a mirror which flips one image relative to the other. Additionally as noted the the EMCCDs flip the image between conventional and EM readout and this needs to be taken account of. Refactoring this functionality definitely needs testing on the real systems before being merged into the main branch.

[carandraug]

[...] The settings are being used on system in Oxford as this is the only current way to set the transform at cockpit startup. [...]

I really don't think this is true or nor that it is what Cockpit is doing. This is about removing Camera.set_setting("transform", ...) and related from the settings dict. This is not about removing the ability of setting/getting transform via Camera.set_transform and Camera.get_transform. As far as I see, Cockpit is using Camera.set_transform so this should be fine. This does not break setting transform on depot.conf (because that uses Camera.set_transform).

The effect this should have on Cockpit is that transform will no longer be listed when clicking on the "settings" button of a camera.

[dstoychev]

This sounds sensible and I do not think it is going to break any of the systems in Oxford. We lose the ability to change the transform dynamically, but this is a whole separate issue for Cockpit and not often used feature anyway.

[iandobbie]

This needs to be checked. I am pretty sure that the code path being used goes via the settings dict and not the camera.set_transform.

It is also very useful to expose the transform list in the setting menu when configuring the system as it makes it very easy to get the correct transform by trial and error.

Sent from my iPhone

On Jan 6, 2023, at 3:18 PM, dstoychev @.***> wrote:



This sounds sensible and I do not think it is going to break any of the systems in Oxford. We lose the ability to change the transform dynamically, but this is a whole separate issue for Cockpit and not often used feature anyway.

— Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpython-microscope%2Fmicroscope%2Fissues%2F260%23issuecomment-1374086620&data=05%7C01%7Cidobbie1%40jh.edu%7Cfd2f698ac22a424733aa08daf0233af7%7C9fa4f438b1e6473b803f86f8aedf0dec%7C0%7C0%7C638086331370469782%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ldmsGsV9HrXGTtImkkVXH0tB0%2FXv4312nUuEarKKRVY%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABJTBBNJXFTWPFIMW6RLT5TWRB42XANCNFSM6AAAAAATTDGINM&data=05%7C01%7Cidobbie1%40jh.edu%7Cfd2f698ac22a424733aa08daf0233af7%7C9fa4f438b1e6473b803f86f8aedf0dec%7C0%7C0%7C638086331370469782%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=z0l7ecFTDC7edl0DOdOU0msBURx586H4DSAtA6LneBE%3D&reserved=0. You are receiving this because you commented.Message ID: @.***>

[juliomateoslangerak]

I also use the roi definition in the settings menu to adjust the excitation field, but I presume that would be still available as the andor camera settings defining left, top, width and height...

[carandraug]

I also use the roi definition in the settings menu to adjust the excitation field, but I presume that would be still available as the andor camera settings defining left, top, width and height...

Yes. I'm not proposing removing any device specific settings. Although I think it would still nice if Cockpit had some GUI to draw a ROI.

[juliomateoslangerak]

it would still nice if Cockpit had some GUI to draw a ROI. Definitely. I miss very much the menu to choose the ROI from a per-configured list in the depot.

[iandobbie]

I think we need to ensure there is still an easy way to define the ROI programmatically, say from depot.conf. Some have cameras which are bigger than the optical field of view so some of the camera is never used and a predefined ROI is a massive win to save doing it every time you restart.

[carandraug]

I've opened a cockpit issue to discuss the cockpit side details of it.

[dstoychev]

I tested how the transform works on DeepSIM. I confirm that it does not use the settings dictionary and it uses the Camera.set_transform API directly.

[iandobbie]

Thanks Danny, I would say I am just misremembering how it was working. Sorry for the false alarm. I suggest we pull davids code a close the issuse