-
Notifications
You must be signed in to change notification settings - Fork 16
plumb sparse rendering functions #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds sparse rendering functionality for Gaussian Splatting, allowing rendering at specific pixel locations rather than full images. The PR plumbs sparse rendering functions from C++ to Python and adds comprehensive unit tests for both forward and backward passes.
- Adds three new sparse rendering methods:
render_depths_sparse,render_features_sparse, andrender_images_and_depths_sparse - Implements autograd support for sparse rasterization through new
RasterizeGaussiansToPixelsSparseclass - Adds unit tests validating sparse rendering matches dense rendering at specified pixels for both forward and backward passes
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_gaussian_splat_3d.py | Adds new TestGaussianRenderSparse test class with 6 tests validating forward and backward passes for sparse depth, feature, and combined rendering |
| src/python/GaussianSplatBinding.cpp | Adds Python bindings for three new sparse rendering methods and renames two existing sparse methods for consistency |
| src/fvdb/detail/ops/gsplat/GaussianRasterizeForward.cu | Adds PrivateUse1 template specialization for sparse rasterization forward pass |
| src/fvdb/detail/ops/gsplat/GaussianRasterizeBackward.cu | Adds PrivateUse1 template specialization for sparse rasterization backward pass |
| src/fvdb/detail/ops/gsplat/GaussianProjectionForwardUT.h | New header file defining projection forward function with unscented transform support |
| src/fvdb/detail/ops/gsplat/GaussianProjectionForwardUT.cu | New implementation file for projection forward with CUDA, PrivateUse1, and CPU specializations |
| src/fvdb/detail/autograd/GaussianRasterizeSparse.h | New header defining autograd function for sparse Gaussian rasterization |
| src/fvdb/detail/autograd/GaussianRasterizeSparse.cpp | New implementation of autograd forward/backward for sparse rasterization |
| src/fvdb/detail/autograd/GaussianRasterize.h | New header extracted from GaussianProjection.h for dense rasterization |
| src/fvdb/detail/autograd/GaussianRasterize.cpp | New implementation extracted from GaussianProjection.cpp for dense rasterization |
| src/fvdb/detail/autograd/GaussianProjection.h | Refactored to remove rasterization code (moved to GaussianRasterize.h) |
| src/fvdb/detail/autograd/GaussianProjection.cpp | Refactored to remove rasterization code (moved to GaussianRasterize.cpp) |
| src/fvdb/GaussianSplat3d.h | Adds declarations for three sparse rendering methods and helper function sparseRenderImpl |
| src/fvdb/GaussianSplat3d.cpp | Implements sparse rendering methods and sparseRenderImpl using new autograd classes |
| src/CMakeLists.txt | Updates build configuration to include new autograd source files |
| fvdb/gaussian_splatting.py | Adds Python wrapper methods for sparse rendering with support for both Tensor and JaggedTensor inputs |
| fvdb/_fvdb_cpp.pyi | Adds type stubs for new C++ sparse rendering methods |
| fvdb/init.pyi | Exports ProjectionType and ShOrderingMode enums |
Comments suppressed due to low confidence (5)
tests/unit/test_gaussian_splat_3d.py:2292
- The error messages in these assertions say "Sparse depth render" but the test is for sparse features rendering. The error messages should say "Sparse features render" instead.
tests/unit/test_gaussian_splat_3d.py:2409 - The error messages in these assertions say "Sparse depth render" but the test is for sparse features and depths rendering. The error messages should say "Sparse features and depths render" or similar to match the test context.
tests/unit/test_gaussian_splat_3d.py:2447 - Missing
.clone()call: The gradient forlogit_opacitiesshould be cloned like the other gradients to prevent it from being affected by the subsequentzero_()call on line 2451. This should besparse_logit_opacities_grad = self.gs3d.logit_opacities.grad.clone().
tests/unit/test_gaussian_splat_3d.py:2213 - Missing
.clone()call: The gradient forlogit_opacitiesshould be cloned like the other gradients to prevent it from being affected by the subsequentzero_()call on line 2217. This should besparse_logit_opacities_grad = self.gs3d.logit_opacities.grad.clone().
tests/unit/test_gaussian_splat_3d.py:2330 - Missing
.clone()call: The gradient forlogit_opacitiesshould be cloned like the other gradients to prevent it from being affected by the subsequentzero_()call on line 2334. This should besparse_logit_opacities_grad = self.gs3d.logit_opacities.grad.clone().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Francis Williams <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (4)
tests/unit/test_gaussian_splat_3d.py:2409
- Incorrect error message: The message states "Sparse depth render does not match dense depth render" but this test is comparing feature+depth renders (from
render_images_and_depths_sparseandrender_images_and_depths), not just depth renders. The message should be "Sparse feature+depth render does not match dense feature+depth render at specified pixels" or similar.
tests/unit/test_gaussian_splat_3d.py:2287 - Variable name mismatch: This variable is named
dense_depth_pixelsbut should be nameddense_features_pixelssince it contains features fromrender_images, not depth values. The error message in the assertion below also refers to "depth" but should refer to "features".
tests/unit/test_gaussian_splat_3d.py:2292 - Incorrect error message: The message states "Sparse depth render does not match dense depth render" but this test is comparing feature renders (from
render_features_sparseandrender_images), not depth renders. The message should be "Sparse feature render does not match dense feature render at specified pixels".
tests/unit/test_gaussian_splat_3d.py:2404 - Variable name mismatch: This variable is named
dense_depth_pixelsbut should be nameddense_features_pixelssince it contains features (including depth) fromrender_images_and_depths, not just depth values. The error message in the assertion below also refers to "depth" but should refer to "features".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Francis Williams <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Francis Williams <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Francis Williams <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
tests/unit/test_gaussian_splat_3d.py:2407
- The variable name
dense_depth_pixelsis misleading in this context. The code is testing feature rendering (which returns images/RGB data), not depth values. The variable should be renamed todense_feature_pixelsordense_image_pixelsto accurately reflect what it contains.
tests/unit/test_gaussian_splat_3d.py:2412 - The error message "Sparse depth render does not match dense depth render at specified pixels" is incorrect. This test is comparing feature/image rendering, not depth rendering. The message should be updated to something like "Sparse feature render does not match dense feature render at specified pixels".
tests/unit/test_gaussian_splat_3d.py:2524 - The variable name
dense_depth_pixelsis misleading. The code is testingrender_images_and_depthswhich returns features with depth as the last channel, not just depth values. The variable should be renamed to something likedense_features_and_depths_pixelsordense_feature_pixelsto accurately reflect the content.
tests/unit/test_gaussian_splat_3d.py:2529 - The error message "Sparse depth render does not match dense depth render at specified pixels" is incorrect. This test is comparing features+depth rendering (images_and_depths), not pure depth rendering. The message should be updated to something like "Sparse features+depth render does not match dense features+depth render at specified pixels".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
tests/unit/test_gaussian_splat_3d.py:2407
- Variable name
dense_depth_pixelsis misleading. This should bedense_features_pixelssincerender_imagesreturns features/RGB data, not depth data. The variable name doesn't match what it actually contains.
tests/unit/test_gaussian_splat_3d.py:2412 - Error message says "Sparse depth render does not match dense depth render" but this test is for
render_features_sparse, which renders features/RGB data, not depth data. The error message should say "Sparse features render does not match dense features render".
tests/unit/test_gaussian_splat_3d.py:2524 - Variable name
dense_depth_pixelsis misleading. This should bedense_features_pixelssincerender_images_and_depthsreturns features with depth as the last channel, not just depth data. The variable name doesn't accurately reflect what it contains.
tests/unit/test_gaussian_splat_3d.py:2529 - Error message says "Sparse depth render does not match dense depth render" but this test is for
render_images_and_depths_sparse, which renders features with depth as the last channel, not just depth. The error message should say "Sparse features+depth render does not match dense features+depth render" or similar.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/unit/test_gaussian_splat_3d.py:2328
- Missing
.clone()call on gradient assignment. Line 2328 should besparse_logit_opacities_grad = self.gs3d.logit_opacities.grad.clone()to be consistent with lines 2325-2327 and to avoid issues when the gradient is later zeroed out on line 2332.
tests/unit/test_gaussian_splat_3d.py:2570 - Missing
.clone()call on gradient assignment. Line 2570 should besparse_logit_opacities_grad = self.gs3d.logit_opacities.grad.clone()to be consistent with the pattern used for other gradients (lines 2567-2569, 2571-2572) and to avoid issues when the gradient is later zeroed out on line 2576.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
tests/unit/test_gaussian_splat_3d.py:2408
- The variable name
dense_depth_pixelsis misleading - it should bedense_features_pixelssince this test is for rendering features, not depth. The error messages also incorrectly refer to "depth" when they should refer to "features".
tests/unit/test_gaussian_splat_3d.py:2536 - The variable name
dense_depth_pixelsis misleading - it should bedense_features_pixelssince this test renders features and depths together, not just depth. The error messages also incorrectly refer to "depth" when they should refer to "features" or "features and depths".
tests/unit/test_gaussian_splat_3d.py:2328 - Inconsistent use of
.clone()forsparse_logit_opacities_grad- it's missing.clone()on line 2328 while all other gradient variables use.clone()on lines 2325-2327. This could cause issues if the gradient is modified later, though in this specific case it appears to work since the gradient is only used for comparison.
tests/unit/test_gaussian_splat_3d.py:2570 - Inconsistent use of
.clone()forsparse_logit_opacities_grad- it's missing.clone()on line 2570 while all other gradient variables use.clone()on lines 2567-2572. This could cause issues if the gradient is modified later, though in this specific case it appears to work since the gradient is only used for comparison.
tests/unit/test_gaussian_splat_3d.py:2403 - The error message refers to "Sparse depth render" but this test is for rendering features, not depth. The message should say "Sparse features render" instead.
tests/unit/test_gaussian_splat_3d.py:2531 - The error message refers to "Sparse depth render" but this test is for rendering features and depths together, not just depth. The message should say "Sparse features and depths render" instead.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
swahtz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for plumbing this functionality up through the python API and putting in the pytests. We should @harrism some time to review on his workweek to be able to chat about the naming of the functions or potentially merging the public interface to sparse/dense render into the one function.
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
Signed-off-by: Francis Williams <[email protected]>
swahtz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for fixing up those notes.
plumbs sparse rendering functions from C++ to Python and adds unit tests for forward and backward