docs: fix marching_cubes return type (unique vertex indices, not normals)#653
Conversation
…als) Fixes openvdb#422 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
|
|
The two red CUDA 13.0 jobs look unrelated to this docs-only change. Both fail at import time with No code change should be needed here. Happy to leave it open for a re-run once the CUDA 13.0 build pipeline is reconciled with the test environment. |
swahtz
left a comment
There was a problem hiding this comment.
Thanks for your contribution @mvanhorn and fixing up this issue. The tests should pass now if you want to just merge main into your PR branch.
While the docstring for unique_vertex_ids is technically accurate, it just defines the output using the variable name and is not really helpful for the user to understand what the meanings are of those vertex IDs.
As I look at what unique_vertex_ids actually is, I think we should expand the scope of this Issue to improve what is being returned here. The shape of this output for the single function is (N, 3) and for the batch function it is (B,-1,3) and the meaning of the rows per vertex is something like[batch_idx, voxel_a, voxel_b] which identifies the grid edge on which the vertex was interpolated, where voxel_a and voxel_b are flat voxel indices of the two endpoint voxels. batch_idx is entirely repetitive information because the batch is encoded by the JaggedTensor implicitly in the batch function and has no meaning (or is 0) in the single function. We could just change this output to be shape (N,2).
So if you want to keep this PR scoped as-is, I think we should change the name of this return value to something like vertex_edge_keys (the uniqueness was just an internal implementation detail) and change our documentation something like (update shape for the batch version as appropriate):
"Edge keys used to deduplicate mesh vertices, dtype torch.int64, shape (N, 3). Each row [batch_idx, voxel_a, voxel_b] identifies the grid edge on which the vertex was interpolated, where voxel_a and voxel_b are flat voxel indices of the two endpoint voxels (voxel_a >= voxel_b). This can be used to map mesh vertices back to the grid edges and voxels they originated from."
If you want to just open an Issue for the return shape difference and just land this docstring change, that's fine, or happy if we want to do both in this one PR since they're both small changes.
|
The failing CUDA |
The issue with CI is related to your branch not having merged the latest from |
Summary
marching_cubesreturns three values: vertices, faces, and an int64 tensor of unique vertex indices (the output oftorch.unique_dimatMarchingCubes.cu:357-385). The docstrings infvdb/grid.py,fvdb/grid_batch.py, andfvdb/functional/_meshing.pydescribed the third return as "vertex normals", which the implementation does not produce.Why this matters
The docs led users to expect a normals tensor and write code like
vertices, faces, normals = .... The third value is actuallyunqVertIdxfrom the unique-vertex pass, which has a completely different shape (int64) and meaning.Changes
fvdb/grid.py,fvdb/grid_batch.py,fvdb/functional/_meshing.py- update the docstring ofmarching_cubesat all four call sites to describe the third return as "unique vertex indices (int64)".Fixes #422