Open jo-mueller opened 8 months ago
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
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 :warning: |
napari_process_points_and_surfaces/__init__.py | 66.66% | 1 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi @haesleinhuepf ,
I think keeping the connected_components
functional with the current vedo version could be a bit tricky. I see two options:
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
- Replicate the
connected_components
functionality with what is still there in vedo. This would boil down to re-engineering the connected components labelling using theconnected vertices
function from vedo somehow like this:
Yes, that sounds great! Does the code-snippet work already?
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 👍
@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 the review, here goes. Sorry about the __main__
thing, I really forget to remove it all the time 🙈
Sorry about the
__main__
thing, I really forget to remove it all the time 🙈
Give pytest -k my_specific_test_to_try
a try ;-)
@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.
@haesleinhuepf so....can this go in? 👼
Happy new year btw :)
Edit: I think with this going through, tests for #83 should also pass.
I have to find the time to test this carefully... Will not happen before February I presume...
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.
Fixes #87
Changes
mesh.points()
withmesh.vertices
mesh.faces()
withmesh.cells
mesh.CenterOfMass
withmesh.center_of_mass
mesh.origin
connected_components_labeling
in vtk since it is deprecated on the vedo sidesplit_mesh
and_split_mesh
.SurfaceTuple
, which render nicely in notebooks and which is exposed to the nppas APILayerDataTuple
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 newmesh.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 aList[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 🤔