Skip to content

[Bug Fix] Improve binary search handling in JIdxForJOffsets#325

Merged
harrism merged 4 commits into
openvdb:mainfrom
iYuqinL:patch-1
Nov 19, 2025
Merged

[Bug Fix] Improve binary search handling in JIdxForJOffsets#325
harrism merged 4 commits into
openvdb:mainfrom
iYuqinL:patch-1

Conversation

@iYuqinL

@iYuqinL iYuqinL commented Nov 8, 2025

Copy link
Copy Markdown
Contributor

Refactor binary search logic to handle cases with duplicate offsets correctly.

The original jidx from joffsets binary search can not handle the case that when joffsets has same values.
For example, joffsets is [0, 10, 10, 40], when idx = 10, the original code will wrongly set jidx to -1.

Refactor binary search logic to handle cases with duplicate offsets correctly.

Signed-off-by: Yuqin Liang <YuqinLiangX@gmail.com>
@iYuqinL iYuqinL requested a review from a team as a code owner November 8, 2025 14:25
@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 8, 2025

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@iYuqinL iYuqinL changed the title Improve binary search handling in JIdxForJOffsets [Bug Fix] Improve binary search handling in JIdxForJOffsets Nov 8, 2025
@harrism harrism added this to fVDB Nov 11, 2025
@harrism harrism added the bug Something isn't working label Nov 11, 2025

@fwilliams fwilliams left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the fix! I added a few comments about conforming to codestyle. You can see all the style fixes here: https://github.com/openvdb/fvdb-core/actions/runs/19194298081/job/55129073233?pr=325

If possible, could you also add a unit test in Python? You can add it to tests/unit/test_jagged_tensor.py

Comment thread src/fvdb/detail/ops/JIdxForJOffsets.cu Outdated
Comment thread src/fvdb/detail/ops/JIdxForJOffsets.cu Outdated
Comment thread src/fvdb/detail/ops/JIdxForJOffsets.cu Outdated
Comment thread src/fvdb/detail/ops/JIdxForJOffsets.cu Outdated
@iYuqinL

iYuqinL commented Nov 12, 2025

Copy link
Copy Markdown
Contributor Author

@fwilliams @harrism hello, I have formatted the code using clang-format as you suggested.
And I have also added a unit test function to tests/unit/test_jagged_tensor.py, which named test_jidxforjoffsets.

@harrism harrism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the great contribution! Can you please add a C++ test? Should only need a couple lines in an existing test.

Comment thread tests/unit/test_jagged_tensor.py
@iYuqinL iYuqinL requested a review from harrism November 12, 2025 11:48
@iYuqinL

iYuqinL commented Nov 12, 2025

Copy link
Copy Markdown
Contributor Author

@harrism hello, I have add two unit tests to src/tests/JaggedTensorTest.cpp file, named JIdxFromJOffsetsCPU and JIdxFromJOffsetsCUDA. They test the jidx_from_joffsets function on CPU device and CUDA device.

I also run the unit test in my local env, without the kernel modification, the JIdxFromJOffsetsCUDA runs fail as expected.
iShot_2025-11-12_19 42 06

with the kernel modification, both JIdxFromJOffsetsCPU and JIdxFromJOffsetsCUDA can run successfully.
iShot_2025-11-12_19 45 38

@harrism harrism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this excellent PR! We really appreciate your contribution @iYuqinL.

Comment thread src/tests/JaggedTensorTest.cpp
Comment thread src/tests/JaggedTensorTest.cpp
Signed-off-by: Yuqin Liang <YuqinLiangX@gmail.com>
Signed-off-by: Yuqin Liang <YuqinLiangX@gmail.com>
Signed-off-by: Yuqin Liang <YuqinLiangX@gmail.com>
@iYuqinL

iYuqinL commented Nov 13, 2025

Copy link
Copy Markdown
Contributor Author

@harrism hello, I discovered that my previous commits weren't GPG signed.
I re-push the signed commits, but it seems you need to review them again to get them through the merge process.

@harrism

harrism commented Nov 19, 2025

Copy link
Copy Markdown
Contributor

Thank you for this fix and the new test. We look forward to your future contributions!

@harrism harrism merged commit 597eda1 into openvdb:main Nov 19, 2025
32 checks passed
@github-project-automation github-project-automation Bot moved this to Done in fVDB Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants