add existing points layer

[thanushipeiris]

This PR tries to solve #11 but is very inelegant.

It doesn't seem magicgui has implemented Optional for napari layers so instead I added two compulsory layer points reference_pts and moving_pts.

Case 1: When there are no points layers on the screen, no reference and moving points layers can be selected. Affinder functions like normal. https://user-images.githubusercontent.com/54516770/151280006-b8ac697a-5999-4e92-9591-c030ba0ca8f7.mp4

Case 2a: When points layers are present on the screen, reference and moving pts layers must be specified. Affinder will then add existing points to these layers. https://user-images.githubusercontent.com/54516770/151280118-08528c6f-9bce-4621-9757-12ad4a372cdf.mp4

Case 2b: If the user has points layers on the screen but does not want to use any of them in affinder (i.e. they'd like to start new points layers), they would need to manually add empty points layers and specify them in the reference and moving pts dropdowns (this is what the Optional magicgui support would have prevented). https://user-images.githubusercontent.com/54516770/151280461-2c01de19-5c47-447f-8827-8651cb7d6058.mp4

I'm not sure how urgently we want this feature or if we're happy to wait for whenever magicgui figures out how to handle Optional[napari.layers.Layer] type declarations. If we do want it, I will add documentation in the readme letting people know about the three cases above.

[codecov-commenter]

Codecov Report

Merging #50 (5dd07c3) into main (6610fdc) will increase coverage by 0.45%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   80.99%   81.45%   +0.45%     
==========================================
  Files           5        5              
  Lines         121      124       +3     
==========================================
+ Hits           98      101       +3     
  Misses         23       23              

Impacted Files	Coverage Δ
src/affinder/affinder.py	91.46% <100.00%> (+0.32%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6610fdc...5dd07c3. Read the comment docs.

[thanushipeiris]

Perhaps when the user provides their own points layer and it has no points, I should change the colour of the points to blue if it's in the reference layer and orange if it's in the moving layer. This would make it a bit easier for Case 2b (above) as the user won't need to change the colour of the points themselves.

[jni]

@thanushipeiris are you sure that Optional[] doesn't work? It should, see napari/magicgui#227, and if it doesn't, that's a bug. Do you think you could produce a minimal reproducible example and raise an issue in magicgui?

[jni]

Hmm, indeed, just verified it. I made the following changes to your PR:

diff --git a/src/affinder/affinder.py b/src/affinder/affinder.py
index 3bb5ec6..ea27e22 100644
--- a/src/affinder/affinder.py
+++ b/src/affinder/affinder.py
@@ -90,10 +90,11 @@ def close_affinder(layers, callback):
         )
 def start_affinder(
         viewer: 'napari.viewer.Viewer',
+        *,
         reference: 'napari.layers.Layer',
-        reference_points: 'napari.layers.Points',
+        reference_points: Optional['napari.layers.Points'] = None,
         moving: 'napari.layers.Layer',
-        moving_points: 'napari.layers.Points',
+        moving_points: Optional['napari.layers.Points'] = None,
         model: AffineTransformChoices,
         output: Optional[pathlib.Path] = None,
         ):

Then ran:

from skimage import data, transform
import napari
import numpy as np

image0 = data.camera()
image1 = transform.rotate(image0[100:, 32:496], 60)

viewer = napari.Viewer()
l0 = viewer.add_image(image0, colormap='green', blending='additive')
l1 = viewer.add_image(image1, colormap='magenta', blending='additive')
l2 = viewer.add_points([[0, 0]])

qtwidget, widget = viewer.window.add_plugin_dock_widget(
        'affinder', 'start_affinder'
        )

napari.run()

There are no widgets shown for the pre-existing points layers.

@tlambert03 should this work? Do we need to make modifications to how napari registers types to enable optional categorical types to work here? Or are we doing something wrong?

[jni]

(For convenience, the changes above are in the branch existing-points-optional, together with the failing example at examples/basic-example.py.)

[jni]

@thanushipeiris so, to answer your question, yes, this is a good workaround, and tbh it probably covers the most common use cases, but I think I would prefer to wait until we can implement the feature in magicgui.

Another alternative is to make two functions, start_affinder, and start_affinder_with_existing_points, with the first one containing only the points creation, then calling out to the second one. But honestly, that doesn't seem so great either.

[jni]

@thanushipeiris I had a chat with @tlambert03 who found a bug in how magicgui deals with the mixture of ForwardRefs (type annotations that are strings) and Optional:

from typing import Optional
from magicgui.widgets import create_widget
from napari.layers import Layer

assert create_widget(annotation=Optional[Layer])._nullable  # works
assert not create_widget(annotation=Layer)._nullable  # works
assert create_widget(annotation=Optional['napari.layers.Layer'])  # forwardref, fails

Our options are:

• import the types to annotate
• wait a few days and Talley thinks he can have it fixed.

[thanushipeiris]

Okay cool, which one would be better? I'm happy to do either. Personally think the Optional['napari.layers.Layers'] is more readable though.

[jni]

Yeah, with the imports I was having issues with circular imports at one point anyway, so let's just wait for @tlambert03's fix to land.

[tlambert03]

Close!

https://github.com/napari/magicgui/pull/362

[jni]

I've incorporated your commits in #54, which further fixes things a bit for npe2, so this can be closed. Thanks @thanushipeiris!