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

New simulated stage and camera device. #144

Closed iandobbie closed 4 years ago

iandobbie commented 4 years ago

We need a simulated stage controller that will also simulate a camera to return subsections of a large image as a demo for cockpit and microscope.

iandobbie commented 4 years ago

I have started to write a simulated stage device to back this up. I don't think this will be hard. However I couldn't work out at all how controller devices are written and work. I tried to look at the priorproscan and the Aurox but couldn't make head or tail of either. Can anyone give me some hints as to how I would write a controller that would pick up the test stage device I am writing and a simulated camera that returns a subset of a large tile scan to simulate a tile scanning stage.

carandraug commented 4 years ago

This is mixing a few thing together and maybe it's better to have them separate. The final aim is to have test devices that can provide a more real microscope experience. For this we need a testcamera that has a very large image and return a subsection based on the stage position.

To achieve this what we need is:

  1. test stage
  2. test camera aware of the stage position

so technically, we don't need a test controller for it. If those classes exist, then maybe something like his:

stage = TestStage(some_limits)
camera = TestCameraWithImage(stage, image_fpath)

At that point, I guess we could have a TestController that puts those two lines together, but maybe it's not worth it:

controller = TestControllerWithImage(image_fpath)

For what is worth, I do have a "test-controller" branch that implements a TestController. It takes as arguments the arguments to construct the devices it controls. It gets a bit messy.

So maybe start by only implementing a test stage, then make cockpit use microscope stages.

iandobbie commented 4 years ago

Yes this was going to be my approach. write a test stage device (seems quite simple, only need about 10 properties/functions). then think about the test camera. I have also been trying to find an available large mosaic image but haven't found a nice one yet. Will update once I get a test stage working.

carandraug commented 4 years ago

I have also been trying to find an available large mosaic image but haven't found a nice one yet.

My current internet connection won't allow me to search, but maybe something on the tissue IDR? https://idr.openmicroscopy.org/tissue/

By the way, what about having multiple light sources and change the returned image based on the channel?

iandobbie commented 4 years ago

Good suggestion with the idr. I had a look and failed to be able to download one large image (17,000x16,000). I will have more of a look.

iandobbie commented 4 years ago

I am struggling with the DummyStage. I don't know how to create this property..

@property @abc.abstractmethod def axes(self) -> typing.Mapping[str, StageAxis]:

My stage has ...

class DummyStage(devices.StageDevice): def init(self, kwargs): super().init(kwargs) self.xaxis=DummyStageAxis('x',(0,1000)) self.yaxis=DummyStageAxis('y',(0,1000)) self.axes={'x': self.xaxis,'y': self.yaxis}

but this generates the error...

TypeError: Can't instantiate abstract class DummyStage with abstract methods _on_shutdown, axes, initialize

The complete code is on https://github.com/iandobbie/microscope/tree/TestStage

iandobbie commented 4 years ago

Current code slightly better as I implemented most of the missing functions. Now returns an error

TypeError: Can't instantiate abstract class DummyStage with abstract methods axes

I don't understand this as the axes is a property not a methods isnt it?

carandraug commented 4 years ago

I don't understand this as the axes is a property not a methods isnt it?

Kind of. You need to implement it as a method but with the @property decorator, like so:

    @property
    def axes(...):
iandobbie commented 4 years ago

Moving towards a working function. Now have methods that return the properties...

def position(self):
    return (self._position)

unfortunately the get position function then needs to be return {name: axis.position() for name, axis in self.axes().items()}

notice the extraneous brackets for function calls. How do we get these as properties and not methods?

iandobbie commented 4 years ago

Seemed to have worked it out. Please see if I have done something stupid

iandobbie commented 4 years ago

I am currently downloading a 9.5k X 9.5k image which we could use, obviously with credit given.

I think I have a working stage but its going to be next week before I do any more. I am interviewing all day tomorrow.

carandraug commented 4 years ago

I have given a quick review of the test stage and axis:

iandobbie commented 4 years ago

I have given a quick review of the test stage and axis:

  • The axis class does not need a name argument and indeed it never makes use of it, so I think it can be removed.

Could be but might be useful to be able to know which axis is which, otherwise you just have a function pointer. I guess fi you only ever access the axis through the stage.

  • It's missing type annotations.

Will update this

  • It might make sense to pass an AxisLimits instance directly to the stage axis class instead of a tuple.

Ok, will change this.

  • The computation of initial position at middle of range is only correct if one of the limits is zero. Examples: limits of (-10, -5) sets mid position at -2.5 which is outside the range; limits of (5, -3) sets position at 4

This is just sloppy coding.

  • An axis limits is a property but not annotated as such. Will update

  • Most test devices are named TestX, the only exceptions being DummyDSP and DummySLM which don't even have an ABC define yet. For sake of consistency, maybe name them TestStage and TestStageAxis?

I started with this name, then looked at the two functions above which are DummyDSP and DummySLM so changed it. I will change it back.

iandobbie commented 4 years ago

I have done the suggested updates, except left the name property for now as until I do a cockpit implementation I don't want to remove it, I am worried I might need it.

Can you please check the type annotations are sensible.

iandobbie commented 4 years ago

I have now been trying to implement a cockpit side for this test stage and the microscope abstract base class totally re-invents the wheel. Why is this?

1) Microscope has never had connectivity awareness, yet now we have named axis? Why is it not just a list of axis? This level of config has aways been in cockpit not microscope.

2) Cockpit uses move_rel and move_abs all of a sudden microscope has move_to and move_by.

3) The new names tuple AxisLimits means whole swathes of cockpit code need to be re-written.

What is the advantage of these changes? Is your prior stage based on this base class?

carandraug commented 4 years ago

Can you please check the type annotations are sensible.

Done. They're fine.

Microscope has never had connectivity awareness, yet now we have named axis? Why is it not just a list of axis? This level of config has aways been in cockpit not microscope.

The only stage device microscope had before this was the Linkam stage which also returns a dict when reading stage position and so also requires named axis. A list of axis, like cockpit does, causes problems when going beyond xyz and even then can get messy with a need for placeholders and checks for non existent axis.

I would expect the only configuration needed for a MicroscopeStage is an axis map. While keeping cockpit limited to three axis, I think the simplest is to have 'x-axis-name', 'y-axis-name', and 'z-axis-name' config entries. An alternative is to have a single config entry hose value is a list up to three elements (but I think the three separate configs is clearer, simpler for z stages, and easier to expand with more axis in the future).

Cockpit uses move_rel and move_abs all of a sudden microscope has move_to and move_by.

The new names tuple AxisLimits means whole swathes of cockpit code need to be re-written.

I don't understand why you would need to touch any of the existing cockpit code. You should only need to write a MicroscopeStage class on cockpit/devices/microscopeDevice.py . The MicroscopeStage should instantiate and return a PositionerHandler for each axis which is what the rest of cockpit is then meant to use.

Following the same logic, using move_rel/abs instead move_to/by, shouldn't require changes to any existing cockpit code. The linkam stage that Mick wrote, and predates the Stage ABC, also uses move_to. I don't have a preference either way, it's just one character shorter and does not require abbreviations. I also thought of having move for relative and move_to for absolute which was even shorter but in the end found I had to think twice what move actually did.

Also with AxisLimits class. It should never leave the MicroscopeStage class, and so should not require any swathe of cockpit code to be re-written. Using namedtuples instead of just tuples makes for clearer code and enables static code analysis. We do the same for ROI and Binning.

iandobbie commented 4 years ago

You are of course right. The previous comment caught me this morning after not sleeping well. I did get it working, and worked out these things.

Still not sure about the axis labels but actually the current mechanism was relatively easily used to get it all to work. Only outstanding hurdle is graphics issues due to the stage size/shape assumptions in the macrostageXY panel (see https://github.com/MicronOxford/cockpit/issues/585)

iandobbie commented 4 years ago

I have also started to work on a new test camera mode that adds two setting mosaic_xpos and _ypos and returns an image based on these. I know it needs more work but I would like to try to create a controller to talk to both stage and camera and get the stage to return camera images based on its position. David can you point me at the simplest code that implements a controller?

Still to add

1) Would be nice to return different channels of the image if different filters are selected, or different cameras used.

2) No edge detection, relatively easy to add, probably in the stage module, or just map the stage to slightly smaller than the mosaic, or return blank outside useful region

3) scale factor, would be better to have a scale factor and be able to return different pixel sizes, simulating different objective magnifications.

carandraug commented 4 years ago

David can you point me at the simplest code that implements a controller?

Sure. Here's implementation for three different controller instances:

import microscope.devices
import microscope.testsuite.devices

class ControllerWithNothing(microscope.devices.ControllerDevice):
    @property
    def devices(self):
        return {}

class ControllerWithOneTestCamera(microscope.devices.ControllerDevice):
    def __init__(self, **kwargs):
        self._devices = {
            'dummy camera 1' : microscope.testsuite.devices.TestCamera(),
        }

    @property
    def devices(self):
        return self._devices

class ControllerWithTwoDevices(microscope.devices.ControllerDevice):
    def __init__(self, **kwargs):
        self._devices = {
            'camera' : microscope.testsuite.devices.TestCamera(),
            'laser' : microscope.testsuite.devices.TestLaser(),
        }

    @property
    def devices(self):
        return self._devices
carandraug commented 4 years ago

I was looking at the TestStage to get it merged, and I have two minor things:

  1. on the move methods there's a sleep call with the comment "moving instantly breaks things often". What things break because of this? Nothing on microscope side should be breaking because of this.

  2. I think the limits and name of the individual axis should be an argument for the TestStage constructor.

  3. Did you found in the end that you really need the name property in the TestStageAxis? If not, then I would remove it. If it really is required, then it should be added to the ABC anyway.

I can make the changes above, just let me know, and then I will merge the test stage.

iandobbie commented 4 years ago
  1. on the move methods there's a sleep call with the comment "moving instantly breaks things often". What things break because of this? Nothing on microscope side should be breaking because of this.

You are correct this is a Cockpit issue, especially the mosaic. We should have this delay on the cockpit side.

  1. I think the limits and name of the individual axis should be an argument for the TestStage constructor.

Good suggestion, then the cockpit side can implement an arbitrary sized and shaped stage.

  1. Did you found in the end that you really need the name property in the TestStageAxis? If not, then I would remove it. If it really is required, then it should be added to the ABC anyway.

I have no concrete use for this but I worry that without this the only way to identify a specific axis is to know which object it is, this seems pretty brittle.

I can make the changes above, just let me know, and then I will merge the test stage.

Please do

iandobbie commented 4 years ago

Once you have the stage fixed and merged I suggest you close this issue and we open another one for the controller device of a merged stage and mosaic camera

carandraug commented 4 years ago

I have made the changes mentioned above, removed trailing whitespace, added some documentation, squashed the commits and merged.

I suggest you close this issue and we open another one for the controller device of a merged stage and mosaic camera

Closing this as fixed then. I suggest you also separate those in two. Instead of a test controller you should have a TestCamera that can take an image and TestStage as arguments to its constructor. The test controller can be added later.

iandobbie commented 4 years ago

Well the mosaic based test camera is already coded, but probably needs more features. I will create these two issues.