Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove settings from base classes #260

Open
carandraug opened this issue Jan 6, 2023 · 13 comments
Open

remove settings from base classes #260

carandraug opened this issue Jan 6, 2023 · 13 comments

Comments

@carandraug
Copy link
Collaborator

carandraug commented Jan 6, 2023 •
edited
Loading

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 (d5e6c39), and "readout mode" from Camera (953d977).

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 ee7842a)

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 added a commit that referenced this issue Jan 6, 2023
@carandraug
Camera: remove "transform" setting (#260)
ee7842a 
We have `Camera.set_transform` and `Camera.get_transform` methods to
control the client transform.  There is also a readout transform (only
used by PVCamera at the moment).  The transform setting exposes the
merge of the those two.  We want settings to be device specific stuff
and not general camera stuff that should be a ABC method.

This commit removes the transform setting from Camera.
@carandraug
Copy link
Collaborator Author

I've now pushed ee7842a 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
Copy link
Collaborator

iandobbie commented Jan 6, 2023 via email

@iandobbie
Copy link
Collaborator

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
Copy link
Collaborator Author

[...] 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
Copy link
Collaborator

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
Copy link
Collaborator

iandobbie commented Jan 6, 2023 via email

@juliomateoslangerak
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator

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 carandraug mentioned this issue Jan 9, 2023
Closed
@carandraug
Copy link
Collaborator Author

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

@dstoychev
Copy link
Collaborator

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@carandraug @iandobbie @juliomateoslangerak @dstoychev