Fix vedo api

[jo-mueller]

Fixes #87

Changes

• replaced all calls to mesh.points() with mesh.vertices
• replaced all calls to mesh.faces() with mesh.cells
• replaced deprecated call to mesh.CenterOfMass with mesh.center_of_mass
• removed deprecated call to mesh.origin
• re-implemented connected_components_labeling in vtk since it is deprecated on the vedo side
• Added split_mesh and _split_mesh.
  • The first function returns a list of SurfaceTuple, which render nicely in notebooks and which is exposed to the nppas API
  • The second function simply calls the first, but returns a list of LayerDataTuple and is only visible through napari.

About the latter one: It's a bit unfortunate that the previous compute_connectivity, which assigned a label to each vertex doesn't exist anymore. The new mesh.split() function returns a list of vedo meshes, for which I don't know any better way to return them to napari other than returning a List[LayerDataTuple]. An option could be to additionally pass an index of a mesh to extract from the split, but the downside is that selecting the index would be a bit ambiguous since you wouldn't know from looking at the mesh which index you'd have to select in order to get the desired object from the mesh 🤔

[haesleinhuepf]

This change seems unrelated to the PR title. Also does it break backwards compatibility?

[jo-mueller]

@haesleinhuepf Thanks for looking at this. I thought about it for a while and came to the conclusion that it really is a breaking change, already from the vedo side. The original compute_connectivity function allowed to retrieve a per-vertex label which indicated to which object it belonged, which we could then nicely display as the vertex color.

In the new version, the id of each vertex isn't public anymore and vedo just returns a list [mesh1, mesh2, ...] of the connected components :/

To answer your question: The function mesh.compute_connectivity() has been renamed in vedo to mesh.split() so I'd consider that as part of an API change, and it has been done so in a breaking way.

[haesleinhuepf]

But we could use the split function to implement what has been there before, correct? I'm asking because I'm using this stuff in projects. Removing it is no option

[jo-mueller]

But we could use the split function to implement what has been there before, correct? I'm asking because I'm using this stuff in projects. Removing it is no option

I'm not sure, tbh. The split function never exposes which vertex belongs to which object. -_-

[haesleinhuepf]

But it gives lists of vertices grouped, no?

[jo-mueller]

It returns a list of meshes. Each mesh consists of vertices [0, n_mesh_i]

[haesleinhuepf]

Hey Johannes @jo-mueller ,

thanks for working on this!

Before merging this, it would be great to turn backwards compatibility breaking changes into deprecations. This reduces bugs in scripts which use our library. In general I tend to avoid backwards compatibility breaking changes whenever possible.

Furthermore, could you please specify the vedo version this is compatible with here:
https://github.com/jo-mueller/napari-process-points-and-surfaces/blob/723ae03653c3af71c8a1c5ba9142c782e5ee2f95/setup.cfg#L42

Thanks!

Best,
Robert

[codecov-commenter]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4b61d89) 72.91% compared to head (0ae5a7b) 73.08%.

Files	Patch %	Lines
napari_process_points_and_surfaces/_vedo.py	87.50%	4 Missing ⚠️
napari_process_points_and_surfaces/__init__.py	66.66%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   72.91%   73.08%   +0.17%     
==========================================
  Files           8        8              
  Lines        1115     1137      +22     
==========================================
+ Hits          813      831      +18     
- Misses        302      306       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jo-mueller]

Hi @haesleinhuepf ,

I think keeping the connected_components functional with the current vedo version could be a bit tricky. I see two options:

• Restrict this PR to only updating the stuff that doesn't touch the connected_components function (e.g., mesh.vertices and mesh.cells, etc). The connected_components function will be tagged as deprecated for some time and we'll (for now) pin the vedo version here to the last version where this function still worked.
• Replicate the connected_components functionality with what is still there in vedo. This would boil down to re-engineering the connected components labelling using the connected vertices function from vedo somehow like this:

  labels = [None] * len(list_of_points_in_surface)
  
  current_label = 1
  for idx in range(len(list_of_points_in_surface)):
    connected_vertices = vedo_mesh.connected_vertices(idx)
    labels[connected_vertices]  = current_label
  
    # remove connected points from list of points in surface
    current_label += 1

  This function could be kept in the future under the same name (connected_components) and the split function would be a new function to use or not.

Let me know what you think

[haesleinhuepf]

• Replicate the connected_components functionality with what is still there in vedo. This would boil down to re-engineering the connected components labelling using the connected vertices function from vedo somehow like this:

Yes, that sounds great! Does the code-snippet work already?

[jo-mueller]

Yes, that sounds great! Does the code-snippet work already?

Just pulled it out of my fingers, not really ^^" I think editing the list of which is being iterated may become a bit of a mess. Using a while-loop instead that checks whether there are still elements left in list_of_points_in_surface would be a safer way to go.

I'll write it clean in the coming days 👍

[jo-mueller]

@haesleinhuepf , I re-implemented the deprecated compute_connectivity function in vtk. Turned out that vedo_mesh.connected_vertices(idx) only returns the 1-hop neighbors of the queried vertex, which is horribly slow to do as above. The vtk-based implementation is much faster. I updated the PR description above for all changes.

[haesleinhuepf]

Thanks for working on this @jo-mueller . Just two minor clean up requests. We're almost there.

Thanks!

[jo-mueller]

@haesleinhuepf Thanks for the review, here goes. Sorry about the __main__ thing, I really forget to remove it all the time 🙈

[haesleinhuepf]

Sorry about the __main__ thing, I really forget to remove it all the time 🙈

Give pytest -k my_specific_test_to_try a try ;-)

[jo-mueller]

@haesleinhuepf

Give pytest -k my_specific_test_to_try a try ;-)

I know, I do use it, but in case a test fails find it very helpful to step through the test with the debugger line by line, for which you need a __main__ in the test file.

[jo-mueller]

@haesleinhuepf so....can this go in? 👼

Happy new year btw :)

Edit: I think with this going through, tests for #83 should also pass.

[haesleinhuepf]

I have to find the time to test this carefully... Will not happen before February I presume...

[jo-mueller]

Hi @haesleinhuepf , pinging you here again - do you mind if I merge this? The bug with the changed API in the repr_html is causing some errors in the exercises in our current bio-image analysis lecture. I'd also take over maintenance for future issues if you don't mind.