napari: Error when returning named shapes/surface layer multiple times (Dock widget)

Running a dock widget multiple times (i.e. pressing the call_button) will cause an error with named shapes (and also surface, not shown) output layers.

Code

Minimal dock widget to reproduce the issue.

@magic_factory(call_button=True)
def widget() -> List[napari.types.LayerDataTuple]:
    polygons = [np.array([[11, 13], [111, 113], [22, 246]])]
    # return [(polygons, dict(shape_type='polygon'), 'shapes')] # this works
    return [(polygons, dict(name='Test', shape_type='polygon'), 'shapes')]

@napari_hook_implementation
def napari_experimental_provide_dock_widget():
    return widget

Expected behavior

Not to throw an error. Would have expected that the layer is either replaced (as happens for image and labels layers) or a new layer is added with a name suffix (e.g. Test [1]) as happens if the layer is not named.

Environment

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 23 (21 by maintainers)

Most upvoted comments

If something is passed to layer.data then it overwrites anything in layer.shape_type

I like this, I think it makes the most sense.

I think this is a reasonable approach to fix the use case and provide more flexibility overall, and I think the List[Tuple(Array, str)] is quite readable as well. @jni ?

DragaDoncila on Apr 1, 2021

One thing to consider is does that mean we’re removing the shape_type from the metadata? Because otherwise if we’re going to support it in both places I can definitely see sources of confusion and/or inconsistent behaviour there.

I think i’d probably keep it, and inside the data setter split out the shape type so that layer.shape_type is always returns List[str] and layer.data always returns List[Array]. If something is passed to layer.data then it overwrites anything in layer.shape_type.

Note in the motivating example above you’d need to now do

def widget() -> List[napari.types.LayerDataTuple]:
    polygons = [np.array([[11, 13], [111, 113], [22, 246]])]
    # return [(polygons, dict(shape_type='polygon'), 'shapes')] # this works
    # return [(polygons, dict(name='Test', shape_type='polygon'), 'shapes')] # this still wouldn't work
    return [((polygons, 'polygon'), dict(name='Test'), 'shapes')] # this would now work

which doesn’t look too bad to me …

Another thought is that we should swap the order of line 180 and the for loop lines (181, 182) here https://github.com/napari/napari/blob/102a7e8f845893c874d2b86f9371d41130100b89/napari/utils/_magicgui.py#L180

and then if we additionally go with the approach I propose above

if the number of shapes isn’t changing just replace the actual data (keep shape type/ colors etc the same), if the number of shapes is decreasing trim those other lists from the end, if the number of shapes is growing use the current_* property if we have one or the next best sensible default.

and the optional tuple, then return [(polygons, dict(name='Test', shape_type='polygon'), 'shapes')] will just work without extra thought, and all the special cases can be handled.

I know this discussion has gotten quite involved now, but I do see a way forward that will fix this use case, allow for additional flexibility, and be mostly backwards compatible

sofroniewn on Apr 1, 2021

@sofroniewn on it now

DragaDoncila on May 11, 2021

did #2544 close this?

Yes I think so, combined with #2507

sofroniewn on Apr 16, 2021

all clear from me! 😊

jni on Apr 1, 2021

We do have an internal shape object already (right now is also contains the dims to be displayed, which we’ll want to change) https://github.com/napari/napari/blob/master/napari/layers/shapes/_shapes_models/shape.py

ie, maybe if we decide not to go for a full object, we could go for data being a list of (points, datatype) tuples? I think that is the least intrusive option while still maintaining data/shapetype parity.

I was imagining Array | Tuple(Array, str) | List[Array | Tuple(Array, str)] | Tuple(List[Array], str) as the full set of allowed inputs. I think this would be the most minimal and backwards compatible change we could make. Currently we support Array | List[Array] and I don’t think we’d want to loose that.

I also think most users will have a single shape type per layer most of the time, and we don’t want to make things unnecessarily hard for them

sofroniewn on Apr 1, 2021

Looks like something funky is definitely happening, the error message “three vertices to a rectangle” is relatively deep in the code, not quite sure what would have caused that. We’ll look into it.

sofroniewn on Mar 8, 2021