Fix JaggedTensor.from_*_and_list_ids ldim=2 issue#357
Conversation
…Lists correctly for the ldim==2 case. Add tests for these functions Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
swahtz
added
bug
… list_Ids of dim 2 Fix documentation, add docstrings in JaggedTensor.h Fix checks for the list_ids empty case Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the from_data_indices_and_list_ids and from_data_offsets_and_list_ids factory methods where mNumOuterLists was incorrectly set to the total number of tensors instead of the number of outer lists for ldim==2 JaggedTensors. The fix adds logic to detect when ldim==2 (by checking if mListIdx.size(1) == 2) and correctly computes the number of outer lists by finding the maximum value in the first column of list_ids. Additionally, comprehensive regression tests and improved documentation were added.
- Added input validation checks for the factory methods to ensure proper tensor shapes
- Fixed
mNumOuterListscalculation forldim==2case by usingmax()on the first column oflist_ids - Updated a related usage in the CUDA code to use the correct accessor
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/test_jagged_tensor.py |
Added comprehensive regression tests for both from_data_offsets_and_list_ids and from_data_indices_and_list_ids factory methods, testing both ldim=1 and ldim=2 cases |
src/tests/JaggedTensorTest.cpp |
Updated test data to use proper 2D tensor format for list_ids parameter |
src/fvdb/JaggedTensor.h |
Enhanced documentation for factory methods with detailed examples and parameter descriptions |
src/fvdb/JaggedTensor.cpp |
Added input validation and fixed mNumOuterLists calculation for ldim==2 by computing max of first column in list_ids |
fvdb/jagged_tensor.py |
Updated Python docstrings to match C++ documentation improvements and clarify parameter shapes |
src/fvdb/detail/ops/gsplat/GaussianRasterizeContributingGaussianIds.cu |
Changed check from num_outer_lists() to joffsets().size(0) - 1 to correctly count tensors rather than outer lists |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
…t 2 dimensions (not empty) Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
…vice too Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
Signed-off-by: Jonathan Swartz <jonathan@jswartz.info>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Fix issue where 'from_*_and_list_ids' functions did not set mNumOuterLists correctly for the ldim==2 case. mNumOuterLists was being set to the total number of tensors, not the length of the first set of lists. Added tests for these functions
Fixes #356