python-microscope / microscope

Python library for control of microscope devices, supporting hardware triggers and distribution of devices over the network for performance and flexibility.
https://www.python-microscope.org
GNU General Public License v3.0
69 stars 41 forks source link

Clarity support with pipeline for data processing #124

Closed mickp closed 3 years ago

mickp commented 4 years ago

Addresses issue #121. Needs testing with hardware.

carandraug commented 4 years ago

I have spoken with Mick earilier today in person about this PR and the following is just a summary of that.

This Clarity mixing a ControllerDevice with a FilterWheel should really be some sort of composite device. The problem it tries to solve by being a controller is the same that we have on AO-tools and with live SIM reconstruction (at least in the manner that Marcel envisioned it).

Not mentioned anywhere yet on this PR is that the reasoning behind this design is that we want to avoid passing data around between processes for processing: camera gets image, sends it to the Clarity for processing, Clarity then sends it back to the camera, which finally sends it to the client. Using a controller makes that pretty easy since everything ends up in the same process.

However, we are just delaying the problem and we will eventually need to come up with some interface to composite devices and possibly how to share data between processes (the multiprocessing module has classes for that but there's a whole bunch of issues that we may have to think about, specially since we mainly want to shared numpy arrays and not python arrays).

For now, we need this Aurox interface and it does do what we need it to do. It enables us to delay coming up with a general solution to composite devices.

mickp commented 4 years ago

I've addressed some of the review items, and rebased this onto master. I think the only outstanding issues are:

carandraug commented 4 years ago

whether to use camera.some_parameter=x or camera_kwargs = {'some_parameter': x}

It just occurred to me that we can't use a dot on parameter name, the following will fail because dot can't be used in a keyword:

Clarity(camera.some_parameter=x)

even though this works:

Clarity(**{'camera.some_parameter':x})
mickp commented 4 years ago

So it currently only works because the DeviceServer uses the second method to call the constructor?

On Mon, 6 Jan 2020 at 11:25, Carnë Draug notifications@github.com wrote:

whether to use camera.some_parameter=x or camera_kwargs = {'some_parameter': x}

It just occurred to me that we can't use a dot on parameter name, the following will fail because dot can't be used in a keyword:

Clarity(camera.some_parameter=x)

even though this works:

Clarity(**{'camera.some_parameter':x})

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/MicronOxford/microscope/pull/124?email_source=notifications&email_token=ABHGTL37QU5AIMBZNDHPU3DQ4MIMBA5CNFSM4JV5D3M2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIFF4LI#issuecomment-571104813, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHGTL5YB2FE7NBTBNPD3F3Q4MIMBANCNFSM4JV5D3MQ .

--


Mick Phillips


carandraug commented 4 years ago

So it currently only works because the DeviceServer uses the second method to call the constructor?

Yes.

This point is just about the use of a period on an argument name, it's not about whether passing a dict of arguments to the camera vs passing multiple args marked for the camera.

mickp commented 4 years ago

I think we need another approach for this.

Data processing can modify the shape of returned arrays, yet when we query this, the request goes to the underlying data source which returns the original shape. With the pipeline approach on one method, it's not easy to address this. A better approach might be to take the underlying camera class, dynamically create an augmented class that modifies both the data collection and data shape methods, and serve an instance of that augmented class.

carandraug commented 3 years ago

We have #133 which is a different implementation also from Mick, that addresses some of the issues from this solution. We are not merging this. Closing.