Shapes data does not conform to LayerDataProtocol

gselzer commented 2 years ago

🐛 Bug

Since Shapes is a Layer, its data should fulfill LayerDataProtocol. However, (some) internal ways of populating this don't preserve the required properties

To Reproduce

Steps to reproduce the behavior:

Run the test:

def test_data_protocol():
    shp = Shapes()
    # Add 2-D ellipse
    data = np.ones((2, 2))
    data[:, 0] = [20, 20] # Center
    data[:, 1] = [10, 10] # Axes
    shp.add_ellipses(data)
    # Check for properties
    assert hasattr(shp.data, 'dtype')
    assert hasattr(shp.data, 'shape')

Expected behavior

I would expect that shp.data would have the shape and dtype properties.

Environment

Please copy and paste the information at napari info option in help menubar here:


napari: 0.4.13
Platform: Linux-5.13.0-28-generic-x86_64-with-glibc2.31
System: Ubuntu 20.04.3 LTS
Python: 3.9.10 | packaged by conda-forge | (main, Feb 1 2022, 21:24:11) [GCC 9.4.0]
Qt: 5.12.9
PyQt5: 5.12.3
NumPy: 1.20.3
SciPy: 1.7.3
Dask: 2022.01.1
VisPy: 0.9.4

OpenGL:

GL version: 4.6 (Compatibility Profile) Mesa 21.0.3
MAX_TEXTURE_SIZE: 16384

Screens:

screen 1: resolution 2560x1440, scale 1.0
screen 2: resolution 1920x1080, scale 1.0

Plugins:

Labeling: 0.1.dev9+g4bf5a30
console: 0.0.4
scikit-image: 0.4.13
svg: 0.1.5

tlambert03 commented 2 years ago

yep, shapes and surfaces were noted omissions from the original LayerDataProtocol PR. Shapes are rather heterogeneous, and require some re-working to provide the protocol from https://github.com/napari/napari/pull/2683 ... It would need something like the MultiScale wrapper there

gselzer commented 2 years ago

yep, shapes and surfaces were noted omissions from the original LayerDataProtocol PR

Ah, so they are! Shapes still seems to be from the LayerDataProtocol docstring.

I agree that it is a large undertaking to make it conform - maybe it would just be best in the short term to better specify that it does not conform? Maybe it should either just not use data, favoring some different terminology?

tlambert03 commented 2 years ago

That seems like a reasonable intermediate step. The whole layer data protocol is rather new: it's an acknowledgement that we should at least try to define the methods expected of a given layers's data object. But as a late addition, it still needs some attention

gselzer commented 2 years ago

What do you think of changing the current Shapes.data object then to Shapes.shapeData, and adding throwing an clear NotImplementedError in the Shapes.data getter and setter? That seems preferrable to getting an error saying that the data just doesn't have these attributes.

tlambert03 commented 2 years ago

cc @sofroniewn

sofroniewn commented 2 years ago

What do you think of changing the current Shapes.data object then to Shapes.shapeData, and adding throwing an clear NotImplementedError in the Shapes.data getter and setter? That seems preferrable to getting an error saying that the data just doesn't have these attributes.

While I can understand this - it would be a big breaking change to take away layer.data from shapes, everyone using the layer would have to change. We might just need to bite the bullet and add a .shape and .type

It would need something like the MultiScale wrapper there

Indeed I think it would. I could imagine how this might be done. Maybe we should give it a go? What do @andy-sweet @jni @nclack think?

gselzer commented 2 years ago

What do you think of changing the current Shapes.data object then to Shapes.shapeData, and adding throwing an clear NotImplementedError in the Shapes.data getter and setter? That seems preferrable to getting an error saying that the data just doesn't have these attributes.

While I can understand this - it would be a big breaking change to take away layer.data from shapes, everyone using the layer would have to change.

That's a great point, yeah. For that reason, we probably shouldn't do this.

napari / napari