Acquisition reads from "LC Calibration" tab instead of the most recently run/loaded calibration

[talonchandler]

The following steps illustrate the problem:

• Set "Illumination Scheme" to "5-State"
• "Run Calibration"
• "Acquire Birefringence"
• Set "Illumination Scheme" to "4-State" (without re-running the calibration)
• "Acquire Birefringence" --- fails with a mismatched shape error: ValueError(f'Unsupported image data size. Provide image data is of size: {data_dims}.'

I think recOrder needs to keep it's internal calibration state separate from the GUI. In this case, enter_calib_scheme is run whenever the GUI changes, which updates self.calib_scheme, which is read and used by acquisition_workers.

I might suggest that QLIPP_Calibration gets new attributes that are used to keep track of the latest calibration. With these new attributes, the acquisition_workers can call self.calib_window.calib.calib_scheme instead of the current self.calib_window.calib_scheme.

I have similar recommendations for handling swing and wavelength.

I'm very open to different solutions, too! Thanks @ziw-liu.

[ziw-liu]

QLIPP_Calibration.wavelength and QLIPP_Calibration.swing already exist: https://github.com/mehta-lab/recOrder/blob/78b7bbbb4e7d2fd81f7d1295b8cfddee4e59001c/recOrder/plugin/main_widget.py#L1739-L1741

We can add a similar attribute for calib_scheme and rewire the acquisition behavior. However, the init/set/get operations of these attributes are rather fragmented. Given the needs that surfaced in #262, I wonder if this is a good time to 'do it right', i.e. to gather calibration parameters in its own data structure and clearly define the interfaces. Or we just keep adding to the fragmentation for 0.3.0.

Edit: QLIPP_Calibration.calib_scheme also exists. https://github.com/mehta-lab/recOrder/blob/f84f09d7b800ffea5e0845296b0a837a26712d52/recOrder/calib/Calibration.py#L145

[ziw-liu]

It appears that the acquisition workers do get a QLIPP_Calibration object passed into __init__(): https://github.com/mehta-lab/recOrder/blob/f84f09d7b800ffea5e0845296b0a837a26712d52/recOrder/acq/acquisition_workers.py#L530-L539

But this attribute is never accessed by their methods during acquisition and reconstruction. Switching from something like AcquisitionWorker.calib_window.calib_scheme to the form of AcquisitionWorker.calib.calib_scheme will largely resolve this issue.

However, this will also disable integration testing of almost any functionality of 0.3.0 without a microscope being connected to the computer, where a calibration object is not available. @talonchandler @talonchandler Any idea how to work around this?

[talonchandler]

What sort of integration testing do you think won't work with a calibration object? My (possibly naive) understanding is that most of the integration testing we're doing for 0.3.0 requires a microscope connected to the computer, so I don't think this change will be a big deal.

We currently allow the user to click the "Acquire buttons" without a loaded calibration, but this is arguably a bug. We currently error when the user requests a background without a calibration, and I think we should do the same if the user requests an acquisition without a calibration.

https://github.com/mehta-lab/recOrder/blob/f84f09d7b800ffea5e0845296b0a837a26712d52/recOrder/plugin/main_widget.py#L1904-L1907

[ziw-liu]

I have similar recommendations for handling swing and wavelength.

We have two GUI fields for wavelength: one in the calibration tab and one in the reconstruction tab, and currently the later is used for reconstruction in acquisition workers. It's certainly not the most robust design, and our options are:

1. Leave it as-is, since 'reconstructing with the displayed reconstruction parameters' kind of makes sense.
2. Read the value from the currently loaded QLIPP_Calibration, and use neither of the GUI fields.
3. Since offline reconstruction is now disabled for 0.3.0, we can also remove the wavelength field in reconstruction and use the value from the calibration object.

https://github.com/mehta-lab/recOrder/blob/f84f09d7b800ffea5e0845296b0a837a26712d52/recOrder/acq/acquisition_workers.py#L230-L235

[talonchandler]

An extra wrinkle I just thought of: estimating phase requires a value for the wavelength, and you might try to acquire and reconstruct phase without running or loading a calibration.

Even though "leave it as is" is slightly weird, I think it makes the most sense for this case.

[ziw-liu]

For 'phase-only' there is more 'weirdness' out there: one acquisition worker can read from two different GUI fields to initialize the reconstructor and to check if the reconstructor has changed: https://github.com/mehta-lab/recOrder/blob/f84f09d7b800ffea5e0845296b0a837a26712d52/recOrder/acq/acquisition_workers.py#L265-L267 https://github.com/mehta-lab/recOrder/blob/f84f09d7b800ffea5e0845296b0a837a26712d52/recOrder/acq/acquisition_workers.py#L434-L439

Edit: now I'm leaning towards removing the le_recon_wavelength field

[talonchandler]

Edit: now I'm leaning towards removing the le_recon_wavelength field

Okay I think this makes sense.

So if you try to do a phase-from-BF recon it will read from the calibration tab? And if you try a phase-from-polarization if will read from the loaded QLIPP_Calibration object?

[ziw-liu]

So if you try to do a phase-from-BF recon it will read from the calibration tab? And if you try a phase-from-polarization if will read from the loaded QLIPP_Calibration object?

If you are asking about the behavior after the fix, I'm imagining that they both read from the loaded QLIPP_Calibration object for consistency.

[talonchandler]

Sorry, yes I was asking about behavior after the fix.

both read from the loaded QLIPP_Calibration object

Will there be a default value for QLIPP_Calibration? "Phase from BF" data should be collected without the polarizers in the light path (and we have several users who will have use cases that don't need any polarizers), so how will these users enter a wavelength?

[ziw-liu]

"Phase from BF" data should be collected without the polarizers in the light path

Nice catch! I missed that use case. How about keeping the two wavelength fields and having all the acquisition workers read from the reconstruction tab then?

[talonchandler]

How about keeping the two wavelength fields and having all the acquisition workers read from the reconstruction tab then?

That will work!