Add support for function widgets #1856

sofroniewn · 2020-11-12T00:44:59Z

Description

Following on from #1816, particularly discussion here #1816 (comment) this PR adds the ability to provide functions that we turn into dock widgets using magic gui via an viewer.window.add_magic_function(function, magic, dock) API, where function is the function, magic is a dict that gets passed to the magicgui method and dock is a dict that gets passed to add_dock_widget method.

This PR adds two examples magic_image_arthimetic and magic_parameter_sweep based on respective examples of in the magicgui docs https://magicgui.readthedocs.io/en/latest/

I've made a couple issues on the magic gui around being able to use strings for types, see https://github.com/napari/magicgui/issues/46 and https://github.com/napari/magicgui/issues/47. I'm also curious about the ramifications of the API changes in https://github.com/napari/magicgui/pull/43, but we don't necessarily need to wait to release this until that is merged, though it might be nice.

Once we have this merged it will be much easier to continue with #1816 (or supercede it) as now a plugin will just be able to provide a list of tuples of inputs to add_magic_function.

I havn't added tests yet, but will do so. I think it would be nice to get some review and input on this first though - cc @tlambert03 @GenevieveBuckley @jni

Type of change

New feature (non-breaking change which adds functionality)

codecov · 2020-11-12T01:03:26Z

Codecov Report

Merging #1856 (9eabb9c) into master (947bdbc) will increase coverage by 0.03%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #1856      +/-   ##
==========================================
+ Coverage   79.78%   79.82%   +0.03%     
==========================================
  Files         387      388       +1     
  Lines       33215    33234      +19     
==========================================
+ Hits        26501    26529      +28     
+ Misses       6714     6705       -9

Impacted Files	Coverage Δ
napari/_tests/test_function_widgets.py	`93.33% <93.33%> (ø)`
napari/_qt/qt_main_window.py	`80.05% <100.00%> (+0.28%)`	⬆️
napari/utils/_magicgui.py	`79.62% <100.00%> (+15.34%)`	⬆️
napari/utils/_tests/test_magicgui.py	`100.00% <100.00%> (ø)`
napari/layers/base/base.py	`93.02% <0.00%> (+0.21%)`	⬆️

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 947bdbc...9eabb9c. Read the comment docs.

examples/magic_image_arithmetic.py

examples/magic_parameter_sweep.py

tlambert03

this looks great to me @sofroniewn, exactly how I pictured it. I made a couple small implementation comments. but don't feel strongly on any of them

I've made a couple issues on the magic gui around being able to use strings for types, see napari/magicgui#46 and napari/magicgui#47.

totally agree. will do.

I'm also curious about the ramifications of the API changes in napari/magicgui#43,

The main change as it relates to this PR would be around the .Gui() syntax (which was always a bit weird). Briefly, the new magicgui decorator will return an instance of magicgui.FunctionGui, which itself is a magicgui widget (rather than simply adding a function attribute Gui that points to the widget). outside of napari, that object could be shown just with decorated_function.show() (or decorated_function.show(run=True) to spin up an event loop). To get at the actual QWidget, you would now use .native, much like how vispy does it. so, in this PR, you'd change it to

widget = magicgui(**magic)(function).native

but otherwise, it shouldn't change much here. To get a more complete idea about what is changing from the API perspective, have a look at how the "examples" are changing in https://github.com/napari/magicgui/pull/43/files

examples/magic_image_arithmetic.py

examples/magic_parameter_sweep.py

napari/_qt/qt_main_window.py

tlambert03 · 2020-11-12T16:05:10Z

oh... also, after https://github.com/napari/magicgui/pull/43/

viewer.layers.events.changed.connect(lambda x: widget.refresh_choices())
will become
viewer.layers.events.changed.connect(widget.reset_choices)

sofroniewn · 2020-11-12T16:17:28Z

oh... also, after napari/magicgui#43

viewer.layers.events.changed.connect(lambda x: widget.refresh_choices())
will become
viewer.layers.events.changed.connect(widget.reset_choices)

will each widget always have a refresh_choices() or reset_choices on it? i.e. even if there is no layer argument passed? Does it do a global refresh of all enum / list etc type inputs?

tlambert03 · 2020-11-12T16:25:18Z

will each widget always have a refresh_choices() or reset_choices on it? i.e. even if there is no layer argument passed?

not all widgets, but all CategoricalWidgets ... (a widget will be categorical if it is an Enum or was provided with choices parameter.)

Does it do a global refresh of all enum / list etc type inputs?

it depends on how the categorical widget was created. In this case though, a parameter annotated as a napari Layer will have choices set to the get_layers function (in napari.utils._magicgui). So reset_choices will basically just call that function (which will grab layers in the layerlist)

sofroniewn · 2020-11-12T16:28:02Z

not all widgets, but all CategoricalWidgets ... (a widget will be categorical if it is an Enum or was provided with choices parameter.)

Do I then need to add a check to see if the function/ attribute exists or is it always safe to try and connect in the way I'm doing?

tlambert03 · 2020-11-12T16:30:51Z

not all widgets, but all CategoricalWidgets ... (a widget will be categorical if it is an Enum or was provided with choices parameter.)

Do I then need to add a check to see if the function/ attribute exists or is it always safe to try and connect in the way I'm doing?

ohhh, right! good point :) yeah, I think we'll need to check. but actually, this concept probably belongs in napari.utils._magicgui more than it does here. in any case, let's just remember to revisit this after the new magicgui

sofroniewn · 2020-11-12T16:50:23Z

ohhh, right! good point :) yeah, I think we'll need to check. but actually, this concept probably belongs in napari.utils._magicgui more than it does here. in any case, let's just remember to revisit this after the new magicgui

For now I'll check and then make an issue to follow-up

tlambert03 · 2020-11-12T16:59:09Z

For now I'll check and then make an issue to follow-up

until the magicgui API changes, all widgets will have a refresh_choices method
... and I believe it should be safe to call it even if none of the subwidgets have registered choices (it will just do nothing).

jni

I agree with almost everything @tlambert03 wrote. The broad strokes here though are super great, and I think @sofroniewn will know how to wrap this up, so I'm providing my approval pending the discussed changes.

sofroniewn · 2020-11-15T02:26:43Z

I agree with almost everything @tlambert03 wrote. The broad strokes here though are super great, and I think @sofroniewn will know how to wrap this up, so I'm providing my approval pending the discussed changes.

Ok great sounds like we're pretty close. Out of curiosity @tlambert03 do you have guestimate on a timeline for how long it would take to finish https://github.com/napari/magicgui/pull/43? I just looked and we don't actually depend on magicgui yet - I need to add that to this PR - and if possible it might be nice to make our first dependency on a version after that PR has merged, but I don't think that's a requirement.

tlambert03 · 2020-11-15T02:31:46Z

do you have guestimate on a timeline for how long it would take to finish napari/magicgui#43?

the code is mostly done, (just need to add back labels). The main issues now are tests, and updated docs... and that's unfortunately why it's sat there for a while. but I can probably just live with imperfect test coverage for the first . probably a week or two of focused work? (question is when will the focused work come 😂)

sofroniewn · 2020-11-15T02:44:33Z

the code is mostly done, (just need to add back labels). The main issues now are tests, and updated docs... and that's unfortunately why it's sat there for a while. but I can probably just live with imperfect test coverage for the first . probably a week or two of focused work? (question is when will the focused work come 😂)

ok sounds good, we can play it by ear!

jni · 2020-11-17T00:22:35Z

Given https://github.com/napari/magicgui/pull/43 and how much that will change the API, I think we should probably hold off on this until 0.4.2, despite (or perhaps because of!) the massive community excitement about this. In short, I think many people would start using this immediately, and it would become a major backwards-compatibility headache. Let's take a couple of more weeks and get this right? (Or at least, a little more right. 😂)

tlambert03 · 2020-11-17T00:26:23Z

Let's take a couple of more weeks and get this right? (Or at least, a little more right. 😂)

yeah, I think that's a good idea. While the decorator syntax itself won't change too dramatically, the fact that accessing the widget with .Gui() will likely go away (provided everyone likes that?) is probably reason enough to wait. I promise to get on it soon... this week is really busy, but I will try make it a priority. (and wouldn't mind a couple extra reviews of napari/magicgui#43 ... even just casual perusals)

edit: jeez, I'm realizing now that even the original post there is a bit out of date. so if you want to maximize your review efforts, maybe hold off on that too for a week or so

GenevieveBuckley · 2020-11-17T00:34:48Z

Let's take a couple of more weeks and get this right? (Or at least, a little more right. 😂)

Agreed

tlambert03 · 2020-12-26T23:57:48Z

I'm still having problems using strings instead of napari.layers.Image

ahh... this is because magicgui resolves the forward reference as needed, but doesn't actually replace the type annotation. It would be easy enough to fix on the magicgui side (see https://github.com/napari/magicgui/issues/65). Otherwise, we'd also need to resolve them in utils._magicgui, you could use the magicgui function for it like this:

def get_layers(...):
    if layer_type is None:
        gui, layer_type = gui.native, gui.annotation

    # resolve the ref
    if isinstance(layer_type, ForwardRef):
        from magicgui.type_map import _evaluate_forwardref
        layer_type = _evaluate_forwardref(layer_type)

tlambert03 · 2020-12-27T00:15:17Z

I think it's probably for the best to do this on the magicgui side, I made a PR: https://github.com/napari/magicgui/pull/66

tlambert03

This looks great @sofroniewn! The examples definitely make it look even easier now to get some gui goodness into napari. :)

I left some thoughts, but you can take them or leave them. Also, if you want, you can remove the comments from utils._magicgui.get_layers if we're going to depend on magicgui>=0.2.0

napari/_qt/qt_main_window.py

tlambert03 · 2020-12-27T00:41:43Z

also: if you want, I try to get napari/magicgui#63, napari/magicgui#64, and napari/magicgui#65 in and release 0.2.1 tomorrow and you can pin to that instead?

Co-authored-by: Talley Lambert <talley.lambert@gmail.com>

napari/_qt/qt_main_window.py

Co-authored-by: Talley Lambert <talley.lambert@gmail.com>

sofroniewn · 2020-12-27T01:53:49Z

I've moved the auto-connection to the add_dock_widget which is nice as any magicgui added widget will now benefit from it. I did it by looking for reset_choices, let me know @tlambert03 if you have another way in mind (what I like about this is that it doesn't mean we have to import magicgui, but we could do different too, feel free to make a suggested change if you want.

If you want to make a commit to this PR that cleans up utils._magicgui now that we have > 0.2.0 that would be great too, might be safer that you do it rather than me!

also: if you want, I try to get napari/magicgui#63, napari/magicgui#64, and napari/magicgui#65 in and release 0.2.1 tomorrow and you can pin to that instead?

Sounds good, no rush

napari/_qt/qt_main_window.py

tlambert03 · 2020-12-27T01:58:04Z

napari/_qt/qt_main_window.py

+            # Keep the dropdown menus in the widget in sync with the layer model
+            # if widget has a `reset_choices`, which is true for all magicgui
+            # widgets
+            self.qt_viewer.viewer.layers.events.connect(widget.reset_choices)


I'm forgetting at the moment... is layers.events the same as "any event" or did we need to do layers.events.inserted and layers.events.removed?

layers.events will do all of them

Co-authored-by: Talley Lambert <talley.lambert@gmail.com>

tlambert03 · 2020-12-27T18:11:45Z

If you want to make a commit to this PR that cleans up utils._magicgui now that we have > 0.2.0 that would be great too, might be safer that you do it rather than me!

ok, I just cleaned up magicgui._utils.get_layers, and I extended your test a bit to make sure that viewer.layers does indeed stay in sync with the layer choices in the widget.

I figured out what was going wrong in #2044 and submitted fix in #2046.
On the magicgui side, I tweaked a few more things, so if you have time to peek at and approve napari/magicgui#67, napari/magicgui#68, napari/magicgui#69, I can release 0.2.1 and we can pin to that instead.

sofroniewn · 2020-12-29T05:39:50Z

On the magicgui side, I tweaked a few more things, so if you have time to peek at and approve napari/magicgui#67, napari/magicgui#68, napari/magicgui#69, I can release 0.2.1 and we can pin to that instead.

I've approved all 3 PRs, they all look good to me. If you want to make a magicgui 0.2.1rc0 tomorrow I can give that a test before you make the actual release, and then once released I'll update this PR.

into magicgui_function

sofroniewn · 2020-12-29T21:10:00Z

Ok this has been updated to now depend on the latest magicgui 0.2.1 and fixed so all tests should be passing. I also added a comment about forward references and link to https://www.python.org/dev/peps/pep-0484/#forward-references in the examples. @tlambert03 can you look those over (occurs in both examples), feel free to directly edit PR. If you give this one final thumbs up I will then merge!! So exciting to get this in!!!

tlambert03 · 2020-12-29T21:20:06Z

@tlambert03 can you look those over (occurs in both examples)

good idea. I made a slight update to explain that ForwardRefs are mostly necessary if you don't want to import napari. all looks good to me i think! 🎉

sofroniewn · 2020-12-29T22:05:17Z

Ok, great, will merge now!!

add support for magic functions

8c07b0a

sofroniewn added the feature New feature or request label Nov 12, 2020

sofroniewn added this to the 0.4.1 milestone Nov 12, 2020

sofroniewn requested review from jni, tlambert03 and GenevieveBuckley November 12, 2020 00:44

GenevieveBuckley reviewed Nov 12, 2020

View reviewed changes

examples/magic_image_arithmetic.py Show resolved Hide resolved

GenevieveBuckley reviewed Nov 12, 2020

View reviewed changes

examples/magic_image_arithmetic.py Outdated Show resolved Hide resolved

GenevieveBuckley reviewed Nov 12, 2020

View reviewed changes

examples/magic_image_arithmetic.py Show resolved Hide resolved

GenevieveBuckley reviewed Nov 12, 2020

View reviewed changes

examples/magic_parameter_sweep.py Show resolved Hide resolved

tlambert03 reviewed Nov 12, 2020

View reviewed changes

examples/magic_image_arithmetic.py Show resolved Hide resolved

examples/magic_parameter_sweep.py Show resolved Hide resolved

napari/_qt/qt_main_window.py Outdated Show resolved Hide resolved

napari/_qt/qt_main_window.py Outdated Show resolved Hide resolved

jni approved these changes Nov 15, 2020

View reviewed changes

jni modified the milestones: 0.4.1, 0.4.2 Nov 17, 2020

sofroniewn modified the milestones: 0.4.2, 0.4.3 Nov 28, 2020

tlambert03 approved these changes Dec 27, 2020

View reviewed changes

napari/_qt/qt_main_window.py Outdated Show resolved Hide resolved

napari/_qt/qt_main_window.py Outdated Show resolved Hide resolved

sofroniewn and others added 3 commits December 26, 2020 17:44

Apply suggestions from code review

9a40e70

Co-authored-by: Talley Lambert <talley.lambert@gmail.com>

change connection location

ec27284

clean up

87d71be

tlambert03 reviewed Dec 27, 2020

View reviewed changes

napari/_qt/qt_main_window.py Outdated Show resolved Hide resolved

Update napari/_qt/qt_main_window.py

958a176

Co-authored-by: Talley Lambert <talley.lambert@gmail.com>

tlambert03 reviewed Dec 27, 2020

View reviewed changes

napari/_qt/qt_main_window.py Outdated Show resolved Hide resolved

tlambert03 reviewed Dec 27, 2020

View reviewed changes

sofroniewn and others added 2 commits December 26, 2020 18:36

Update napari/_qt/qt_main_window.py

c4ccc68

Co-authored-by: Talley Lambert <talley.lambert@gmail.com>

cleanup get_layers and extend test

32c5909

try making viewer before magicfunc

976385a

sofroniewn mentioned this pull request Dec 29, 2020

register LayerData or purpose-specific type instead of napari.layers.Image as return type with magicgui. #2028

Closed

don't skip test locally

d8cc634

tlambert03 mentioned this pull request Dec 29, 2020

tox tests on github actions #2051

Merged

1 task

sofroniewn added 4 commits December 29, 2020 13:02

use string types

49d9f2e

Merge branch 'master' into magicgui_function

df959a5

bump magicgui 0.2.1

9ea8837

Merge branch 'magicgui_function' of https://github.com/sofroniewn/napari

9676dda

into magicgui_function

update example comments

9eabb9c

sofroniewn merged commit eaf9b7b into napari:master Dec 29, 2020

tlambert03 mentioned this pull request Jan 5, 2021

magicgui.widgets.CategoricalWidget not found in magicgui 0.2.1 pyapp-kit/magicgui#81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for function widgets #1856

Add support for function widgets #1856

sofroniewn commented Nov 12, 2020

codecov bot commented Nov 12, 2020 •

edited

Loading

tlambert03 left a comment

tlambert03 commented Nov 12, 2020

sofroniewn commented Nov 12, 2020

tlambert03 commented Nov 12, 2020

sofroniewn commented Nov 12, 2020

tlambert03 commented Nov 12, 2020

sofroniewn commented Nov 12, 2020

tlambert03 commented Nov 12, 2020

jni left a comment

sofroniewn commented Nov 15, 2020

tlambert03 commented Nov 15, 2020

sofroniewn commented Nov 15, 2020

jni commented Nov 17, 2020

tlambert03 commented Nov 17, 2020 •

edited

Loading

GenevieveBuckley commented Nov 17, 2020

tlambert03 commented Dec 26, 2020 •

edited

Loading

tlambert03 commented Dec 27, 2020

tlambert03 left a comment

tlambert03 commented Dec 27, 2020

sofroniewn commented Dec 27, 2020

tlambert03 Dec 27, 2020

sofroniewn Dec 27, 2020

tlambert03 commented Dec 27, 2020

sofroniewn commented Dec 29, 2020

sofroniewn commented Dec 29, 2020

tlambert03 commented Dec 29, 2020

sofroniewn commented Dec 29, 2020

Add support for function widgets #1856

Add support for function widgets #1856

Conversation

sofroniewn commented Nov 12, 2020

Description

Type of change

codecov bot commented Nov 12, 2020 • edited Loading

Codecov Report

tlambert03 left a comment

Choose a reason for hiding this comment

tlambert03 commented Nov 12, 2020

sofroniewn commented Nov 12, 2020

tlambert03 commented Nov 12, 2020

sofroniewn commented Nov 12, 2020

tlambert03 commented Nov 12, 2020

sofroniewn commented Nov 12, 2020

tlambert03 commented Nov 12, 2020

jni left a comment

Choose a reason for hiding this comment

sofroniewn commented Nov 15, 2020

tlambert03 commented Nov 15, 2020

sofroniewn commented Nov 15, 2020

jni commented Nov 17, 2020

tlambert03 commented Nov 17, 2020 • edited Loading

GenevieveBuckley commented Nov 17, 2020

tlambert03 commented Dec 26, 2020 • edited Loading

tlambert03 commented Dec 27, 2020

tlambert03 left a comment

Choose a reason for hiding this comment

tlambert03 commented Dec 27, 2020

sofroniewn commented Dec 27, 2020

tlambert03 Dec 27, 2020

Choose a reason for hiding this comment

sofroniewn Dec 27, 2020

Choose a reason for hiding this comment

tlambert03 commented Dec 27, 2020

sofroniewn commented Dec 29, 2020

sofroniewn commented Dec 29, 2020

tlambert03 commented Dec 29, 2020

sofroniewn commented Dec 29, 2020

codecov bot commented Nov 12, 2020 •

edited

Loading

tlambert03 commented Nov 17, 2020 •

edited

Loading

tlambert03 commented Dec 26, 2020 •

edited

Loading