Repository navigation
Conversation
Signed-off-by: ShiroKSH <kushidashiro@gmail.com>
|
@Caenorst - could you take a look at the equivolume batch-broadcasting fix in this PR when you have a chance, or route it to the right reviewer? I can make any requested changes or add tests. Thanks! |
|
Hi maintainers, this PR has been open for over two weeks. When you have a moment, I would appreciate a review. Thank you. |
Caenorst
left a comment
There was a problem hiding this comment.
Apologies for the delayed answer. Thank you for the contribution, good catch with the issue, can you just revise the tests as suggested and I'll be happy to merge.
| expected = torch.tensor([[2.2898e-15], [1.5422e-09]]) | ||
| assert torch.allclose(tetmesh.equivolume(tetrahedrons, pow=4), expected, rtol=1e-4, atol=1e-18) | ||
|
|
||
| def test_equivolume_batched_meshes(self): |
There was a problem hiding this comment.
This is exposing a caveat in the original test_equivolume because B == T. I think we should just fix test_equivolume to have a different batch size, maybe just increment B. It's better than making new test I think.
Summary
Root cause
equivolumereduced tetrahedron volumes to a tensor of shape(B,), then reshaped it to(1, B)before subtracting it from volumes shaped(B, T). This raised a broadcasting error whenB != Tand mixed means between meshes whenB == T.The mean now has shape
(B, 1), so each mesh is compared with its own mean volume.Validation
TestTetMeshMetricschecks pass in an isolated CPU PyTorch environmentpy_compilepass for the changed files