Bugfix: IndexError slicing Surface with higher-dimensional vertex_values

[aganders3]

Description

This fixes an off-by-one error on Surface layers extent data, which was causing an IndexError slicing that dimension.

Type of change

• Bug-fix (non-breaking change which fixes an issue)

References

closes #5620
Disussed in community meeting on 2023-03-15

How has this been tested?

• all tests pass with my change (though I had to change two of them)
• thanks to @brisvag who tested to make sure the mesh was in the correct spot wrt an image

Final checklist:

• My PR is the minimum possible 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
• If I included new strings, I have used trans. to make them localizable.
  For more information see our translations guide.

[brisvag]

Actually, one thing I haven't checked: how does it behave with higher dimensional vertex data, if at all?

[aganders3]

The limits for the slider for the vertex data are calculated separately (just above this) and appended. So I think it just works? I have tested and it seemed okay to me but again I'm not too familiar with this code and want to make sure it is working as expected with multiple layers.

[codecov]

Codecov Report

Merging #5635 (b5a7caf) into main (8a793b9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5635   +/-   ##
=======================================
  Coverage   89.85%   89.86%           
=======================================
  Files         611      611           
  Lines       51669    51669           
=======================================
+ Hits        46428    46432    +4     
+ Misses       5241     5237    -4     

Impacted Files	Coverage Δ
napari/layers/surface/_tests/test_surface.py	100.00% <100.00%> (ø)
napari/layers/surface/surface.py	87.19% <100.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[aganders3]

Hmm - even on main I can't get a 4D Surface to behave as I thought it would. I'm going to convert this to draft while I play with that.

[aganders3]

Okay I spent some time and understand the extra dims on the vertices a bit more. I think this is ready for review.

Here's some sample code that will split a cow in half and assign (3, 3) random sets of vertex_values:

from vispy.io import imread, load_data_file, read_mesh

import numpy as np

mesh_path = load_data_file('spot/spot.obj.gz')
vertices, faces, *_ = read_mesh(mesh_path)

v = np.pad(vertices, ((0, 0), (2, 0)))
v[np.where(vertices[:, 0] < 0), 0] = 1
values = np.random.random((3, 3, len(v)))
viewer.add_surface((v, faces, values))

So in this screenshot slider 2 is the extra dim on the vertices, and sliders 1 and 0 are the extra dims on the values.

Edit: #4334 I think would be really useful here

[brisvag]

Looks good to me then :) pity that black is doing one of its weird refactors :/

[aganders3]

Yes I was tempted to reformat that, but oh well :/

[brisvag]

Sorry @aganders3 , forgot about this one. Failures may be unrelated, but I'm not sure. Rerunning tests.

[psobolewskiPhD]

This one (pyside6, 3.10 ubuntu)
https://github.com/napari/napari/actions/runs/4545923589/jobs/8027552735?pr=5635#step:8:270
is this issue: #5657
So unrelated. the other one is the random dims hang?

[brisvag]

Thanks @psobolewskiPhD for checking; let's mark as ready to merge then :)

[aganders3]

Thanks to both of you for following up!