Add markers layer by kevinyamauchi · Pull Request #21 · napari/napari

kevinyamauchi · 2018-10-26T05:29:33Z

What does this PR do?

To resolve #20, @sofroniewn and I are working on implementing vispy Markers as a Layer. This is a work in progress and not ready to merge, but comments and suggestions are more than welcome!

The Markers layer takes a list of coordinates for the marker positions and a dictionary of keyword arguments to pass to the set_data() method of the vispy Marker visual and overlays them in a Layer. The Layer updates according to the current indices selected by the sliders.

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Some thoughts:

Currently, the coordinates dimensions are assumed to be in the same order as the sliders. Eventually, it would be nice to have some sort of labels associated with the axes so we can be sure they are mapped correctly (could be related to Pass xarray to napari-gui for autolabeling sliders #14 ).
I am not sure how compatible this will be with the forthcoming image stack rotation, etc. I suspect we will be able to just swap axes on the marker coordinates, but we'll have to see.
There is probably a more efficient way to get the right marker coordinates from the list based on the indices returned from the sliders.
Currently, the individual markers in a given Layer cannot be controlled independently. I am not sure if this is an issue or not.
The markers don't work with ranges or projections (e.g., if we have a MIP). This should probably be implemented before merging.

Example usage

There is a notebook (test_markers.ipynb) to test the markers layer (we will nuke it before merging).

# Create a viewer with a random 5D image (e.g., [x, y, z, c, t])
rand_im = gui.imshow(np.random.rand(10, 10, 5, 3, 2))

# Create a list of marker coords
marker_list = np.array([[5, 3, 0, 0, 0],
                        [2, 2, 0, 0, 0],
                        [4, 9, 0, 0, 0],
                        [5, 4, 1, 1, 1],
                        [7, 3, 5, 2, 2]])

# Create some keywward arguments to pass to the vispy markers 
marker_args = {'edge_color': 'black', 'face_color': 'green', 'size': 20, 'edge_width': 2}

# Create the layer with markers
rand_im.add_marker(marker_list, marker_args)

# We can also change the marker styling by updating marker_args
rand_im.layers[1].marker_args = {'edge_color': 'black', 'face_color': 'blue', 'size': 20, 'edge_width': 2}

Final Checklist:

My PR is the possible minimum work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

pep8speaks · 2018-10-26T05:29:36Z

Hello @kevinyamauchi! Thanks for updating the PR.

There are no PEP8 issues in the file gui/layers/__init__.py !
There are no PEP8 issues in the file gui/layers/_markers.py !

Comment last updated on November 01, 2018 at 16:29 Hours UTC

kevinyamauchi · 2018-10-26T05:37:44Z

@sofroniewn, here's the first go. Let me know what you think!

royerloic · 2018-10-26T20:32:50Z

That's great!
Some comments:

Currently, the coordinates dimensions are assumed to be in the same order as the sliders. Eventually, it would be nice to have some sort of labels associated with the axes so we can be sure they are mapped correctly (could be related to Pass xarray to napari-gui for autolabeling sliders #14 ).

That's the correect way of doing things, I am against having labels change the order somehow... would get very messy and difficult to interpret...

I am not sure how compatible this will be with the forthcoming image stack rotation, etc. I suspect we will be able to just swap axes on the marker coordinates, but we'll have to see.

Any movement of the camera is only the buisness of the camera... But you canplace the
layers however you prefer. One thing to pay attention is how to 'stack' layers correctly. this means you need to provide the bounding box. We can discuss this again together.

There is probably a more efficient way to get the right marker coordinates from the list based on the indices returned from the sliders. Currently, the individual markers in a given Layer cannot be controlled independently. I am not sure if this is an issue or not. The markers don't work with ranges or projections (e.g., if we have a MIP). This should probably be implemented before merging.

We have to think about this, slicing could be done with any object that intersects with that slice, max projection would extend that to an interval in a trivial way with semantics very similar to what happens with projection of voxel images... It might require some more thinking of how exactly we do that. One solution is to internally sort the objects in special lists that are used to synthesise the vispy visual on the fly. More thinking needed for sure.

Note: I think we should keep this 'demo' code somewhere -- and not just nuke it, this is invaluable as a guide. I like to distinguish 'tests' from 'demos'. Demos explain how to use something and provide an interactive demosntration. Tests are automated and can run on CI. Both are important!

sofroniewn · 2018-10-26T21:22:24Z

This looks great! Nice job @kevinyamauchi. I've been giving the PR a demo and here are some thoughts:

Should the function be add_marker or add_markers? I'd lean towards add_markers as vispy uses the plural.
Should all the layer.marker_args be available as properties on the layer i.e. layer.edge_color. This is currently done in the image layer with image.clim an example.
Do we want to provide a list of allowed symbols / colors like is done with the colormaps property on images?
Sizes in vispy takes an array - but it doesn’t do what I expect, thought it would take an array of length number of markers with size for each marker and size them appropriately. I guess this is a vispy thing, but curious if anyone has any insight here.
Changing the layer opacity doesn’t seem to be doing what I expect - it just makes things dark. Again this could be a vispy issue as flagged in the image layer PR, but wanted to note it here too.
The scaling property (change in size with zoom) doesn’t do what I expect or what I would like. It can be set to true or false in vispy but I don't really see a difference. I find this behvior (where it stays the same pixel width on my screen irrespective of zoom) highly frustrating and very limiting. Not sure if I'm just getting the settings wrong.
There has been some discussion in slack about where the dots get shown on a pixel level / whether the (0,0) is at the corner or the center. I think many of us are in agreement that the center should be at (0,0) as is done in matplotlib, but I agree that does not seem to be the case in vispy, and so in order to get the expected matplotlib like behaviour we currently are adding an 0.5 pixel offset to the marker coordinates. This seems highly undesirable, but I guess a fix requires a change at the vispy level.
Agreed on the discussions around slicing and max-projection. When the max-proj. stuff gets added to the main repo and we see how it is implemented for images we can make the same implementation for markers.
Agreed keeping the demo code would be great - we can create a folder for examples and put notebooks in there. I know notebooks can be a pain in repos, but others at czi have some experience with autoconverting between notebooks and .py files in a way that makes versioning easier. I can ask them for help if we want to go down that route.
I think one of the large outstanding question for me now is how we want to think about markers / lines / polygons (and even others that are accessible with vispy like, box, cube, ellipse etc.) . Are these all going to become separate layers, are we going to try and unify them? Should they be part of this PR or considered separately? I think some we clearly want, but unclear supporting all is as high a priority, though if easy - why not?

kne42

Excellent work Kevin/Nick (Kevinick?)!

A few comments on API and some speedup suggestions...

gui/layers/_marker.py

kne42 · 2018-10-27T00:51:02Z

gui/layers/_marker.py

+        """
+
+        in_slice_markers = []
+        indices = list(indices)


unnecessary since unlike the image layer we aren't modifying the indices

gui/layers/_marker.py

kne42 · 2018-10-27T01:12:13Z

gui/layers/_marker.py

+        self.refresh()
+
+    @property
+    def marker_args(self):


Would prefer to have properties for each individual argument instead of this, and to keep marker_args "private."

@sofroniewn pointed this out as well and I definitely agree. I implemented this.

gui/layers/_marker.py

sofroniewn · 2018-10-27T18:19:01Z

Nice job adding the properties explicitly. One thing I noticed is that the doc strings for properties aren’t automatically propagated up to add_markers, so if I do add_markers? in the notebook I don't see the info on edge_width say. Is there way to achieve this?

Setting the size now works with an array, but can it also take a list of the same length, just as a convenience?

The size update does not work when scaling=True - everything reverted to same size for me, and scaling=True still doesn’t appear to scale the size of the points as you zoom in the way I would expect / want. I'll try poking around on the vispy side to see what is up. If we can get it working I think it should be the default behavior as I at least find it more intuitive.

Also on other related layer types from kevin

I am inclined to make lines, polygons, etc. their own layers. This way the layer maps directly to the underlying vispy. That being said, if there's a lot of repeated code, maybe the way to go is to create a base "annotation" class or something like that and subclass it for the particular type.

I agree, I like each one having their own layer and as we start investigating adding a couple more we can see if the "annotation" class makes sense and what should go in it. I'd prioritize adding line and polygon next, but they can come in a separate PR after this one

kne42 · 2018-10-27T22:44:42Z

@sofroniewn add_*’s docstring is autogenerated by taking the class’s docstring and replacing the summary, then appending the returns section.

You can specify your own docstring with the kwonly argument doc.

sofroniewn · 2018-10-27T23:07:43Z

With respect to the scaling=True problems I was noting before, the aberrant behavior only occurs at very high zooms / when the pixel width of the marker is similar to the number of pixels displayed on the screen. This was the case for the demo image, but when I moved to more real world images, say 512x512 and markers that were size 3 normal behavior was observed over a large range on zooms, particularly as I zoomed out. I'd suggest then just changing the default to be scaling=True and leaving it. I know this is different from the vispy default but I think it is the behavior more of our users will be expecting.

kne42 · 2018-10-28T16:38:47Z

gui/layers/_markers.py

+        if self._need_display_update:
+            self._need_display_update = False
+
+            self.viewer._child_layer_changed = True


This should actually only be called when information pertaining to the shape/dimensions change. You should separate this from the properties that change how set_data is called.

Ah okay, this makes sense. I'll move it to the data setter method (the only place we change total size of the layer).

gui/layers/_markers.py

kevinyamauchi · 2018-10-31T16:23:40Z

@sofroniewn , thanks for testing and for the feedback! I'm glad the scaling works - maybe we should make the demo image a more realistic size. I had originally made it small so I could count pixels and make sure the markers were getting put in the right spot. Let's go ahead and change the default as you suggested.

With regards to the the size datatype: we should probably add type hints and checks to all of the property setter funcs.

I also agree that it would be nice if the docstrings from the properties could be pushed to the class docstring. I was talking to @AhmetCanSolak and he mentioned the doc system is being developed currently. Perhaps we should wait until that is stabilized before we do anything too fancy.

kevinyamauchi · 2018-10-31T16:29:13Z

In terms of things remaining before merging, I think we need to do the following:

Add type hints for all public methods
Add type checks where necessary.
Improve the class docstring so add_markers? is useful

What do you think @sofroniewn and @kne42 ?

sofroniewn · 2018-10-31T16:48:07Z

Thanks for changing the scaling default to True. I can see pros / cons of a larger demo image - it is nice to count the pixels to check. Either way is fine with me.

I think your list of 3 bullets before merging sounds good. On the doc strings, agreed no point in going overboard if things are in flux.

One minor thing I might add to the list right now if it hasn't been done yet is making the size param also accept a list, as it is nice to type something like [5, 5, 10] without calling array, and also for size to accept an integer, as right now it only accepts a float, so I have to type 5.0.

Otherwise I think this PR will be good to merge, and more advanced features (like max projections etc.) can come in future PRs when those features are ready for the image layer too

sofroniewn · 2018-10-31T16:54:20Z

Also it looks like in PR #27 the image layer file is going to be renamed, we should probably stick with the same convention for the markers file - though maybe simplest to still merge this PR as is and then change both the name of both markers and image in that PR, but I just wanted to flag for everyone. Maybe @AhmetCanSolak can provide more detail if needed or I've gotten it wrong.

AhmetCanSolak · 2018-10-31T18:20:51Z

Yes @sofroniewn . There will some renaming on other files under the layers folder too. If you want to merge go ahead and we might need to refactor them later.

kevinyamauchi · 2018-11-01T17:32:40Z

I added type hints and updated the class docstring so that should be working now. What do you think, @sofroniewn. Good to go?

edit: I also added support for sizes as a list.

sofroniewn · 2018-11-01T17:57:44Z

Just trying it out now. Doc strings look great, and thanks for adding the list / int for the size param. Not sure if I'm being pedantic, but the edge_width can also be an int not just a float as it says in the doc string.

The only problem I'm running into now is that after I've added the markers if I then change their properties in the notebook the gui no longer updates automatically, it as if a call to refresh is missing. If I do something like adjust the sliders it then updates, but it was nicer before when it did it automatically.

gui/layers/_markers.py

kevinyamauchi · 2018-11-01T19:17:41Z

@sofroniewn, sorry, I accidentally broke the update. I just fixed this as well as updated the docstrings.

sofroniewn

All checks out now, update works from notebook, and looks like its ready to merge.

kevinyamauchi · 2018-11-02T02:26:29Z

I think I just made the final changes. Let me know what you think, @kne42 and @sofroniewn (sorry, my last commit "dismissed" your review)!

sofroniewn

Checked it again - looks good to me.

gui/layers/_markers.py

jni · 2018-11-02T16:48:11Z

As a minor quibble, in examples documentation etc, can we use the coordinate convertions from scikit-image? ie tprc,ch instead of xyzct. (I did just realise that columns conflicts with channels. Oh well.)

gui/layers/_markers.py

royerloic · 2018-11-05T21:30:42Z

Guys, what's left here to do? can we merge?
I would like to get dev and master merged asap
and keep moving...

* master: Add markers layer (#21)

…visible The visibilitychanged slot currently updates the title regardless of the visibility. We are seeing a crash when the viewer is shut down and a signal is sent to the visibilitychanged slot. That executes some code which causes a segfault in the python process[1]. This PR changes the visibility changed slot to only update the title bar if the widget is visible. [1] ``` * frame #0: 0x0000000115cc96e4 QtWidgets`QWidgetPrivate::reparentFocusWidgets(QWidget*) + 356 frame napari#1: 0x0000000115cb99cf QtWidgets`QWidget::setParent(QWidget*, QFlags<Qt::WindowType>) + 911 frame napari#2: 0x0000000115cb82c0 QtWidgets`QWidgetPrivate::init(QWidget*, QFlags<Qt::WindowType>) + 688 frame napari#3: 0x0000000115d73e5e QtWidgets`QFrame::QFrame(QFramePrivate&, QWidget*, QFlags<Qt::WindowType>) + 14 frame napari#4: 0x0000000115dc3705 QtWidgets`QLabel::QLabel(QWidget*, QFlags<Qt::WindowType>) + 277 frame napari#5: 0x0000000116503189 QtWidgets.abi3.so`Sbk_QLabel_Init(_object*, _object*, _object*) + 1497 frame napari#6: 0x000000010015f84d Python`wrap_init + 12 frame napari#7: 0x0000000100113e64 Python`_PyObject_FastCallDict + 143 frame napari#8: 0x00000001001b17a2 Python`call_function + 439 frame napari#9: 0x00000001001aa46b Python`_PyEval_EvalFrameDefault + 3078 frame napari#10: 0x00000001001b1ee2 Python`_PyEval_EvalCodeWithName + 1638 frame napari#11: 0x00000001001b281b Python`_PyFunction_FastCallDict + 447 frame napari#12: 0x0000000100113e99 Python`_PyObject_FastCallDict + 196 frame napari#13: 0x0000000100113fa3 Python`_PyObject_Call_Prepend + 131 frame napari#14: 0x0000000100113d1e Python`PyObject_Call + 101 frame napari#15: 0x000000010015f7d1 Python`slot_tp_init + 57 frame napari#16: 0x000000010015c782 Python`type_call + 178 frame napari#17: 0x0000000100113e64 Python`_PyObject_FastCallDict + 143 frame napari#18: 0x00000001001141fa Python`_PyObject_FastCallKeywords + 97 frame napari#19: 0x00000001001b17a2 Python`call_function + 439 frame napari#20: 0x00000001001aa4fb Python`_PyEval_EvalFrameDefault + 3222 frame napari#21: 0x00000001001b28e0 Python`_PyFunction_FastCall + 110 frame napari#22: 0x0000000100113e99 Python`_PyObject_FastCallDict + 196 frame napari#23: 0x0000000100113fa3 Python`_PyObject_Call_Prepend + 131 frame napari#24: 0x0000000100113d1e Python`PyObject_Call + 101 frame napari#25: 0x000000011126f146 libpyside2.abi3.5.14.dylib`PySide::SignalManager::callPythonMetaMethod(QMetaMethod const&, void**, _object*, bool) + 534 frame napari#26: 0x000000011126eb77 libpyside2.abi3.5.14.dylib`PySide::SignalManager::qt_metacall(QObject*, QMetaObject::Call, int, void**) + 519 ```

Fix BaseGraph import

init markers layer

2400942

pep8

d43c242

kne42 mentioned this pull request Oct 27, 2018

Implement workaround for opacity-order bug #22

Merged

kne42 reviewed Oct 27, 2018

View reviewed changes

kevinyamauchi added 4 commits October 26, 2018 19:18

moved demo scripts

7f0cabc

fixed path issue for demo

f09a9a9

fixed get_shape()

e51113e

kira wizardry

d3c24e6

kne42 reviewed Oct 28, 2018

View reviewed changes

kevinyamauchi added 2 commits October 31, 2018 09:08

optimize marker slicing

adc4d4a

fixed viewer.child_layer update

8bdd70d

scaling default

c76c39b

kevinyamauchi added 2 commits October 31, 2018 16:27

updated docstrings

c83d5dd

added type hints

45fd92f

kne42 reviewed Nov 1, 2018

View reviewed changes

gui/layers/_markers.py Outdated Show resolved Hide resolved

fixed property update

1415234

sofroniewn previously approved these changes Nov 1, 2018

View reviewed changes

renamed coords

a6a7936

kevinyamauchi dismissed sofroniewn’s stale review via a6a7936 November 2, 2018 02:21

sofroniewn approved these changes Nov 2, 2018

View reviewed changes

kne42 reviewed Nov 2, 2018

View reviewed changes

gui/layers/_markers.py Outdated Show resolved Hide resolved

gui/layers/_markers.py Outdated Show resolved Hide resolved

gui/layers/_markers.py Show resolved Hide resolved

gui/layers/_markers.py Outdated Show resolved Hide resolved

fixed size setter

c9ec0db

jni reviewed Nov 2, 2018

View reviewed changes

gui/layers/_markers.py Show resolved Hide resolved

kne42 approved these changes Nov 2, 2018

View reviewed changes

kne42 changed the title ~~RFC: Add markers Layer~~ Add markers layer Nov 2, 2018

kne42 merged commit 1ed7a17 into napari:master Nov 2, 2018

This was referenced Nov 3, 2018

Creating examples folder #24

Closed

Choose coordinate convention for docs #37

Closed

sofroniewn deleted the add_markers branch November 4, 2018 20:05

kne42 referenced this pull request in kne42/napari Nov 7, 2018

Merge branch 'master' into merge-master

a2ee762

* master: Add markers layer (#21)

kephale referenced this pull request in kephale/napari Mar 10, 2023

Merge pull request #21 from jwindhager/base-graph

8db39cf

Fix BaseGraph import

Uh oh!

Conversation

kevinyamauchi commented Oct 26, 2018 • edited by kne42 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

Some thoughts:

Example usage

Final Checklist:

Uh oh!

pep8speaks commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on November 01, 2018 at 16:29 Hours UTC

Uh oh!

kevinyamauchi commented Oct 26, 2018

Uh oh!

royerloic commented Oct 26, 2018

Uh oh!

sofroniewn commented Oct 26, 2018

Uh oh!

kne42 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kne42 Oct 27, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kne42 Oct 27, 2018

Choose a reason for hiding this comment

Uh oh!

kevinyamauchi Oct 27, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sofroniewn commented Oct 27, 2018

Uh oh!

kne42 commented Oct 27, 2018

Uh oh!

sofroniewn commented Oct 27, 2018

Uh oh!

kne42 Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

kevinyamauchi Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinyamauchi commented Oct 31, 2018

Uh oh!

kevinyamauchi commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sofroniewn commented Oct 31, 2018

Uh oh!

sofroniewn commented Oct 31, 2018

Uh oh!

AhmetCanSolak commented Oct 31, 2018

Uh oh!

kevinyamauchi commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sofroniewn commented Nov 1, 2018

Uh oh!

Uh oh!

kevinyamauchi commented Nov 1, 2018

Uh oh!

sofroniewn left a comment

Choose a reason for hiding this comment

Uh oh!

kevinyamauchi commented Nov 2, 2018

Uh oh!

sofroniewn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinyamauchi commented Oct 26, 2018 •

edited by kne42

Loading

pep8speaks commented Oct 26, 2018 •

edited

Loading

kne42 left a comment •

edited

Loading

kevinyamauchi commented Oct 31, 2018 •

edited

Loading

kevinyamauchi commented Nov 1, 2018 •

edited

Loading