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.

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

Exposure time units across catkit2 #181

Closed
ivalaginja opened this issue Apr 22, 2024 · 4 comments
Closed

Exposure time units across catkit2 #181

ivalaginja opened this issue Apr 22, 2024 · 4 comments
Assignees
Labels
collaborators Worked on by external collaborator. Might need some extra help on code integration. question Further information is requested

Comments

@ivalaginja
Copy link
Collaborator

So far, all of the camera services in catkit2 have been coded in the camera-native units for the respective exposure time. This seems to be microseconds for all of:

  • catkit2/services/allied_vision_camera/allied_vision_camera.py
  • catkit2/services/flir_camera/flir_camera.py
  • catkit2/services/zwo_camera/zwo_camera.py

As a consequence, the units for all camera exposure times in the service configuration have been microseconds. This means that no unit conversions were happening for camera exposure times anywhere.

The new service for Hamamatsu cameras (PR: #163) uses the same approach in which the native camera units are being used everywhere, including the service configuration.

This means, however, that there would be inconsistent units for camera exposure times across a testbed repo, which can get really confusing. Additionally, the current camera viewer(s) on HiCAT and THD2 contain a hard-coded conversion from microseconds to seconds right now, which makes the Hamamatsu camera incompatible in the current setup. Note how this is only relevant to THD2 at this point in time.

This can be easily fixed on our side to accommodate different units. However, I have been wondering whether it might be useful to unify the units everywhere (e.g. to seconds because SI unit) and have the conversion to the unit used in the respective camera libraries happen in the services. This would be rather invasive for HiCAT in terms of risk (there are very many places where exposure times are handled), but doable since right now it is a single change to go from microseconds to seconds everywhere. I personally also think this would make it easier to recognize the units, and obviously this change should happen together with the addition of docstrings that state the units clearly.

Alternatively, the decision could be made that the standard camera exposure time unit are microseconds across catkit2, in which case I would prefer to adjust the Hamamatsu service to keep consistency. This is a much faster change, but breaks the idea of using SI units in the configurations (which the camera exposure times have not been doing up until now anyway).

Either way, I would be willing to do this @ehpor but need your input on this - which way are you leaning?

@ivalaginja ivalaginja added question Further information is requested collaborators Worked on by external collaborator. Might need some extra help on code integration. labels Apr 22, 2024
@ivalaginja ivalaginja self-assigned this Apr 22, 2024
@ivalaginja
Copy link
Collaborator Author

Even if we want to move to seconds overall, the faster and safer way to do it would be to integrate the Hamamatsu camera with a unit conversion and keep everything in microseconds right now. Then, at a later point, change everything consistently to seconds (even if those kind of issues tend to end up in the backlog indefinitely). It would save me time and I wouldn't have to do an invasive refactor in HiCAT.

@ehpor
Copy link
Collaborator

ehpor commented Apr 22, 2024

I think I'd prefer to keep everything in microseconds for now. There should probably be an issue to convert the camera viewer from seconds to microseconds, or maybe even full SI units, but idk if any of us really like that.

@ivalaginja
Copy link
Collaborator Author

ivalaginja commented Apr 22, 2024

Ok! I'll change this in the Hamamatsu service then. Should I just keep this issue alive for this, or close it? I have found no other issue about exposure time units in the GUI or elsewhere, neither on HiCAT nor on catkit2.

@ehpor
Copy link
Collaborator

ehpor commented Apr 24, 2024

I'd say, close it. The units for the Hamamatsu are separate from this one.

@ehpor ehpor closed this as completed Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collaborators Worked on by external collaborator. Might need some extra help on code integration. question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants