CAVEconnectome / MeshParty

Apache License 2.0
35 stars 16 forks source link

Vtk skeleton refactor #21

Closed fcollman closed 5 years ago

fcollman commented 5 years ago

this contains a refactor of the skeleton code api and my changes to allow for adding edges from the pcg to the mesh graph.

fcollman commented 5 years ago

so i think this is much better now and shows a clearer/cleaner path for storing extra non_face_edges (though i'm happy to consider renaming at this point), and managing all the derived data products that we want cached.

sdorkenw commented 5 years ago

I would be in favor of renaming mesh_edges. Now, seems to be the best time for it.

fcollman commented 5 years ago

ok so renaming mesh edges... here are my thoughts. the concept we want to capture is the set of edges that include all the face edges and any non_face_edges/extra/linking edges that the mesh contains.

two views on this would be...

a) edges should return the merge, and we add face_edges returns only the face edges. I'm not sure what other operations/applications might have people who believe/insist that the edges are linked to faces in a direct way, and this would break those people's code. I do think in terms of names this is the most clear.

b) we have a new name for this merged set of edges. here are some brainstorms along with what i hate about them.

total_edges (not clear what the total is totaling) all_edges (not clear what all means) mergededges (not clear what it is merging) combined_edges (not clear what it is combining.. i'm sensing a theme here) linking_edges (don't like this as it implies it is only the linking edges) face_and_link_edges (verbose and awful) edges_plus_links (maybe the best, but still not good)

sdorkenw commented 5 years ago

My main problem with .edges is that it changes trimesh's default behavior. I think I have code that relies on these edges being actually the edges from the mesh.

linking_edges does not work, but linked_edges might? We could also call it graph_edges because these edges do not represent a mesh anymore.

ceesem commented 5 years ago

What about graph_edges referring to the union of linked_edges (or maybe just link_edges?) and face_edges? The key feature of the combined edge list is in treating the object as a graph as opposed to a mesh, so this would indicate that this is the right edge list to use in that case.

fcollman commented 5 years ago

okay.. i like this. mesh_edges> graph_edges non_face_edges>link_edges edges>edges

no change in behavior from what i have implemented now (graph_edges is the cached vstack of edges and link_edges, and we serialize link_edges separately when we save)

what do we then call the present add_link_edges method? is that fine? or do we need to say add_link_edges_from_graph_server or add_link_edges_from_agglomeration or something else?

sdorkenw commented 5 years ago

add_link_edges is fine with me