spacetelescope / catkit2

Package for controlling testbed hardware.
https://spacetelescope.github.io/catkit2/
BSD 3-Clause "New" or "Revised" License
3 stars 0 forks source link

Exposure time units across catkit2 #181

Closed ivalaginja closed 4 months ago

ivalaginja commented 4 months ago

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:

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: https://github.com/spacetelescope/catkit2/pull/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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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