Clarity support with pipeline for data processing #124

mickp · 2019-12-05T17:35:36Z

Addresses issue #121. Needs testing with hardware.

microscope/devices.py

carandraug · 2019-12-15T17:19:58Z

microscope/testsuite/devices.py

@@ -250,6 +261,7 @@ def initialize(self):
        """
        _logger.info('Initializing.')
        time.sleep(0.5)
+        self._initialized = True


Why not add this attribute during object construction? This would mean that one only neds to check for it's value and not have always also check if the attribute already exists. Also, should it not be set back to False after shutdown?

Perhaps not a good reason, but this would still work if a device fails to
set that member during init. In the TestCamera case, it's a simple
attribute. If we used a similar idea on real hardware, we might want some
other test (with library calls) to test that the devices is correctly
initialized, so _initialized could then be a property.

This change is only being made to TestCamera so there is no what other device could do.

Yes, because I've introduced it here for now, but we might want to move it over to the base Device class if it's useful.

microscope/filterwheels/aurox.py

carandraug · 2019-12-16T12:51:52Z

microscope/devices.py

+               (0, 1): numpy.flipud(numpy.rot90(data, rot)),
+               (1, 0): numpy.fliplr(numpy.rot90(data, rot)),
+               (1, 1): numpy.fliplr(numpy.flipud(numpy.rot90(data, rot)))
+               }[flips]


Not introduced in this change but isn't this indexing of the dict always perfoming all flips and rotations and then we are discarding them all except one? If so, shouldn't we ensure that we only do the transformation that is actually required?

Good point. I assumed lazy evaluation, but maybe that's not the case. Perhaps lambdas would be more appropriate.

It's not evaluated lazily. I'll wrap it with lambda.

Fix included in this PR: cfff4a5.

carandraug · 2019-12-16T12:53:16Z

microscope/devices.py

+        Subclasses should call super()._process_data(data) after doing their
+        own processing."""
+        import functools
+        return functools.reduce(lambda x, f: f(x), self.pipeline, data)


If we have an attribute which is a list of processing steps, why should subclasses do their own processing first instead of just appending to self.pipeline?

Yes - I think we should do one or the other. This requires some thought, though.

Originally _process_data was a method on DataDevices to implement the processing step, and I think that was meant to be pure virtual.

Now we have Devices that are not DataDevices that need to add processing steps, and I addressed that with the pipeline, but that needs super()._process_data() to ensure that the pipeline is executed after the DataDevice does anything it needs to do (e.g. running it through a LUT, type conversions, reshaping arrays).

The DataDevice could just add it's processing to the start of the pipeline, instead, and _process_data would be implemented on DataDevice and just process the pipeline.

See comments in review immediately below this one.

carandraug · 2019-12-16T12:54:25Z

microscope/devices.py

@@ -409,6 +409,8 @@ def __init__(self, buffer_length=0, **kwargs):
        self._acquiring = False
        # A condition to signal arrival of a new data and unblock grab_next_data
        self._new_data_condition = threading.Condition()
+        # A data processing pipeline: a list of f(data) -> data.
+        self.pipeline = []


Should we not make this a private attribute? Also, would be nice to also have type annotations for it.

Maybe it should be private, but then we'd need methods to allow other devices to add their processing steps.

We talked about this at length.

Most (all?) classes derived from DataDevice should probably add to the pipeline rather than override _process_data.

Devices that override _process_data must call super()._process_data().

The docstring on DataDevice._process_data should indicate this clearly.

We could make pipeline private, but that would require methods to add/remove steps from it.

_process_data docstring made more explicit in 1c16359.
CameraDevice made to use pipeline instead of override in 0361a02.
_process_data is now only defined in one place, on the DataDevice class.

carandraug · 2019-12-16T13:01:58Z

microscope/filterwheels/aurox.py

+        cam_kwargs = {}
+        for key in cam_kw_keys:
+            cam_kwargs[key.replace("camera.", "")] = kwargs[key]
+            del kwargs[key]


Would a camera_kwargs argument with the arguments to construct the camera make this cleaner?

def __init__(self, camera=None, camera_kwargs={}, **kwargs): super().__init__(**kwargs) if camera is not None: self._camera = camera(**camera_kwargs) ...

It might make the code a bit cleaner, but then you'd have nested { {} } in the config, and maybe that's not cleaner / more error-prone.

carandraug · 2019-12-16T22:13:53Z

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.

Leaving this unhandled could prevent other devices in the same process (e.g. on a controller) being shut down.

get_all_settings reports the values of problem settings as None. Previously, we considered adding a new entry, __errors__, to the returned dict, but this is adding an item that is not a setting to a dict where everything else is a setting, and would require clients to handle this key as a special case.

Fixed ClarityProcessor import and conversion of UMat to ndarray.

Dict evaulation is not lazy: previously, this code evaluated all allowable transforms before picking the appropriate one. This is now fixed by breaking it down into the rotation transfrom, then passing the result to one function from a dict of flip transform functions.

mickp · 2019-12-19T12:39:16Z

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

whether or not to make pipeline private
whether to use camera.some_parameter=x or camera_kwargs = {'some_parameter': x}

carandraug · 2020-01-06T11:25:51Z

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 · 2020-01-06T11:51:02Z

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 ***@***.***> 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 · 2020-01-06T13:11:03Z

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 · 2020-01-13T11:00:31Z

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 · 2020-09-17T11:45:03Z

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.

mickp mentioned this pull request Dec 11, 2019

Aurox issue on start-up #127

Closed

carandraug reviewed Dec 15, 2019

View reviewed changes

microscope/devices.py Outdated Show resolved Hide resolved

carandraug reviewed Dec 15, 2019

View reviewed changes

carandraug reviewed Dec 16, 2019

View reviewed changes

microscope/filterwheels/aurox.py Outdated Show resolved Hide resolved

carandraug reviewed Dec 16, 2019

View reviewed changes

mickp force-pushed the 121-pipeline branch from 699946e to 5f4240f Compare December 16, 2019 17:19

mickp force-pushed the 121-pipeline branch from 6cc01f7 to 5f4240f Compare December 19, 2019 11:46

mickp added 15 commits December 19, 2019 11:52

Add simple processing pipeline to DataDevice.

9d6019f

Catch exceptions during shutdown.

dc89bca

Leaving this unhandled could prevent other devices in the same process (e.g. on a controller) being shut down.

Don't leave caller waiting for data if camera not enabled.

713fd6d

Clarity sublcasses controller to add its camera.

ab7e9b0

Added processing and calibration to Clarity.

71b4920

Fix logging typo.

79c40ea

Changes to enable client to start calibration.

67b8a72

Aurox needs to call super().initialize.

c5e9a37

Don't start collection thread until hardware is enabled.

c798270

TestCamera throws exceptions if it should be initialized but isn't.

f8543a2

Changes after tests with hardware.

38d0325

Fixed ClarityProcessor import and conversion of UMat to ndarray.

_logger must be assigned before any try..excepts use it.

a74fd0d

Changed logging level for problems disabling devices on shutdown.

2d70e0f

mickp force-pushed the 121-pipeline branch from 5f4240f to 2d70e0f Compare December 19, 2019 12:06

mickp added 3 commits December 19, 2019 12:10

Clarity now returns {} if no camera instead of {'camera': None}

c5e5270

Added Clarity docstrings with controlled devices info.

0c98ba9

Made _process_data docstring more explicit.

1c16359

mickp force-pushed the 121-pipeline branch from d051b66 to 1c16359 Compare December 19, 2019 12:29

Make CameraDevice use pipeline instead of overriding _process_data.

0361a02

This was referenced Jan 13, 2020

2020 01 some fixes #132

Closed

121 augmentor implementation #133

Open

carandraug mentioned this pull request Jul 12, 2020

pass function to device server instead of class with args as a separate argument #154

Closed

carandraug closed this Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarity support with pipeline for data processing #124

Clarity support with pipeline for data processing #124

mickp commented Dec 5, 2019

carandraug Dec 15, 2019

mickp Dec 15, 2019

carandraug Dec 16, 2019

mickp Dec 16, 2019

carandraug Dec 16, 2019

mickp Dec 16, 2019

mickp Dec 16, 2019 •

edited

Loading

mickp Dec 19, 2019

carandraug Dec 16, 2019

mickp Dec 16, 2019

mickp Dec 19, 2019

carandraug Dec 16, 2019

mickp Dec 16, 2019

mickp Dec 19, 2019

mickp Dec 19, 2019 •

edited

Loading

carandraug Dec 16, 2019

mickp Dec 16, 2019

carandraug commented Dec 16, 2019

mickp commented Dec 19, 2019

carandraug commented Jan 6, 2020

mickp commented Jan 6, 2020 via email

carandraug commented Jan 6, 2020

mickp commented Jan 13, 2020

carandraug commented Sep 17, 2020

Clarity support with pipeline for data processing #124

Clarity support with pipeline for data processing #124

Conversation

mickp commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickp Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickp Dec 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carandraug commented Dec 16, 2019

mickp commented Dec 19, 2019

carandraug commented Jan 6, 2020

mickp commented Jan 6, 2020 via email

carandraug commented Jan 6, 2020

mickp commented Jan 13, 2020

carandraug commented Sep 17, 2020

mickp Dec 16, 2019 •

edited

Loading

mickp Dec 19, 2019 •

edited

Loading