Fix HashGrid truncation error with negative coordinates#1281
Fix HashGrid truncation error with negative coordinates#1281cluster2600 wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a single negative-wrapping test with three new hash-grid tests that validate neighbor discovery for negative coordinates, cross-validate against a brute-force reference, and exercise multiple floating-point precisions (grid_dim increased to 64; wrapping-focused test removed). Changes
Sequence Diagram(s)(silently skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds three new test functions — Key changes:
Concern identified:
Confidence Score: 4/5Safe to merge but the test suite loses its only genuine regression test for the floor/trunc fix — reverting the native change would still produce a green suite. The native fix in hashgrid.h is correct and already in the base branch. The new tests document expected behaviour and cover precisions and coordinate ranges well. However, due to the monotonicity-of-trunc cancellation, all new assertions pass with both the old int() and new floor() code, so a P1 regression-coverage gap remains. warp/tests/geometry/test_hash_grid.py — the three new negative-coordinate test functions need redesign or supplementation to provide genuine regression coverage for the floor() fix. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Point at pa (e.g. -0.3)"] --> B{"cell_func(pa/cw)"}
B -- "floor: -1 ✓" --> C["Virtual cell -1\nPhysical cell 63"]
B -- "trunc: 0 ✗" --> D["Virtual cell 0\nPhysical cell 0"]
E["Query from pb (e.g. +0.2), r=0.6"] --> F{"Query lower bound\ncell_func((pb-r)/cw)"}
F -- "floor(-0.4)=-1 ✓" --> G["Search range: [-1, 0]\nPhysical cells: {63, 0}"]
F -- "trunc(-0.4)=0 ✗" --> H["Search range: [0, 0]\nPhysical cells: {0}"]
C --> I{Point found?}
G --> I
I -- "Cell 63 searched ✓" --> J["Count = 2 ✓ (correct)"]
D --> K{Point found?}
H --> K
K -- "Cell 0 searched ✓\n(bug cell == query cell)" --> L["Count = 2 ✓ (same result!)"]
J --> M["Tests CANNOT distinguish\nfixed vs unfixed code"]
L --> M
style D fill:#ffcccc
style H fill:#ffcccc
style M fill:#ffe0b2
Reviews (5): Last reviewed commit: "Merge branch 'main' into fix/hashgrid-ne..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Generate points centred on the origin so half are in negative space | ||
| points = particle_grid(8, 8, 8, (-8.0, -8.0, -8.0), cell_width * 0.25, 0.1) | ||
| points_arr = wp.array(points, dtype=wp.vec3, device=device) |
There was a problem hiding this comment.
Misleading comment — points are not centred on the origin
The comment says "Generate points centred on the origin so half are in negative space", but the call is particle_grid(8, 8, 8, (-8.0, -8.0, -8.0), ...).
particle_grid builds a grid via np.linspace(0, dim, dim) (range [0, 8]) scaled by radius * 2.0 = 1.0, then shifted by lower = (-8.0, …). This yields coordinates in approximately [-8.0, 0.0] — nearly all negative, not half/half straddling zero.
To genuinely centre on the origin the lower bound should be -4.0:
| # Generate points centred on the origin so half are in negative space | |
| points = particle_grid(8, 8, 8, (-8.0, -8.0, -8.0), cell_width * 0.25, 0.1) | |
| points_arr = wp.array(points, dtype=wp.vec3, device=device) | |
| # Generate points centred on the origin so half are in negative space | |
| points = particle_grid(8, 8, 8, (-4.0, -4.0, -4.0), cell_width * 0.25, 0.1) |
The test still exercises negative coordinates correctly and the brute-force comparison remains valid, but the comment/lower-bound mismatch could mislead future readers or mask coverage gaps if the test is used as a template for further work.
There was a problem hiding this comment.
Good catch on the comment wording — you're right that with lower=(-8, -8, -8) and linspace(0, 8, 8) scaled by 1.0, the points span roughly [-8, 0] rather than straddeling the origin symmetrically. The comment is misleading there.
That said, the test's primary purpose is exercising the floor() fix in negative coordinate space and cross-validating against the brute-force reference kernel. Since the entire cluster sits in negative space the coverage for the bug scenario is actually slightly better than a symmetric distribution would give. The brute-force comparison remains valid regardless of the offset.
I'll update the comment to accurately describe the coordinate range in a follow-up to avoid confusing future readers. Cheers.
There was a problem hiding this comment.
Good spot — updated the lower bound to (-4, -4, -4) so the points actually straddle the origin as the comment says. Cheers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
warp/tests/geometry/test_hash_grid.py (1)
363-370: Use repo docstring markup for code identifiers.Please wrap code elements like
int(),floor(), andtest_hashgrid_queryin double backticks to match the Python docstring convention used in this repo.As per coding guidelines, "Use double backticks for code elements in docstrings (RST syntax):
``.nvdb``,``"none"``".Also applies to: 465-469
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/tests/geometry/test_hash_grid.py` around lines 363 - 370, Update the docstring to use reStructuredText code markup by wrapping the inline code elements with double backticks: replace occurrences of int(), floor(), and test_hashgrid_query with ``int()``, ``floor()``, and ``test_hashgrid_query`` (also update the other instances noted at lines 465-469) so the docstring follows the repo's RST convention for code identifiers; keep the surrounding text unchanged and ensure punctuation remains outside the backticks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@warp/tests/geometry/test_hash_grid.py`:
- Around line 363-370: Update the docstring to use reStructuredText code markup
by wrapping the inline code elements with double backticks: replace occurrences
of int(), floor(), and test_hashgrid_query with ``int()``, ``floor()``, and
``test_hashgrid_query`` (also update the other instances noted at lines 465-469)
so the docstring follows the repo's RST convention for code identifiers; keep
the surrounding text unchanged and ensure punctuation remains outside the
backticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6ce80138-54dc-446f-9f08-2c9709dc24f9
📒 Files selected for processing (2)
warp/native/hashgrid.hwarp/tests/geometry/test_hash_grid.py
Replace C++ int() cast (truncation toward zero) with (int)floor() (toward negative infinity) for float-to-int conversion in hashgrid.h. Three locations are affected: 1. Point cell assignment during grid build (line 86) 2. Query lower bounds (lines 150-152) 3. Query upper bounds (lines 155-157) Without this fix, points with negative fractional cell coordinates are assigned to the wrong grid cell, and query bounds that cross the zero boundary miss valid cells. For example, with cell_width=1.0 a point at -0.3 should be in cell -1 but int(-0.3)=0 places it in cell 0. The (int)floor() pattern is already used elsewhere in the Warp native code (noise.h, texture.h, volume.h). Closes NVIDIA#1256 Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
- Run clang-format on hashgrid.h to fix pre-commit.ci failure - Use double backticks for code elements in docstrings per repo convention Signed-off-by: Maxime Kawawa-Beaudan <maxkawab@gmail.com>
cluster2600
force-pushed
the
fix/hashgrid-negative-coord-truncation
branch
from
63cc6ee to
5578a4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/tests/geometry/test_hash_grid.py`:
- Around line 607-615: Remove the stale registration that refers to the
undefined test function: find the add_function_test call registering
"test_hashgrid_negative_wrapping" (associated with TestHashGrid) and delete that
line so only existing functions like test_hashgrid_negative_coordinates,
test_hashgrid_negative_brute_force, and test_hashgrid_negative_multiprecision
are registered; ensure no other references to test_hashgrid_negative_wrapping
remain in the file to avoid NameError on import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 11225c32-d2c0-4477-a868-399e4c637d14
📒 Files selected for processing (1)
warp/tests/geometry/test_hash_grid.py
Correct the lower bound in test_hashgrid_negative_brute_force from (-8, -8, -8) to (-4, -4, -4) so the generated points genuinely straddle the origin, matching the comment that says "half are in negative space". Remove the stale add_function_test registration for the renamed test_hashgrid_negative_wrapping (now test_hashgrid_negative_coordinates), which would raise NameError at import time. Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
1 participant
Summary
hash_grid_querysilently misses valid neighbours when query bounds or point coordinates are negative. The root cause is float-to-int conversion using C++int()cast (truncation toward zero) instead offloor()(toward negative infinity) inwarp/native/hashgrid.h.Three locations are affected:
build(), points with negative fractional cell coords are binned into the wrong cellFor example, with
cell_width=1.0a point at-0.3should be assigned to cell-1butint(-0.3) = 0places it in cell0. A query centred at+0.2with radius0.6has a lower bound of-0.4, which should search cell-1butint(-0.4) = 0skips it entirely.The fix replaces
int(expr)with(int)floor(Type(expr))at all three locations, matching the pattern already used innoise.h,texture.h, andvolume.h.Test plan
test_hashgrid_negative_coordinates— four subcases: cross-zero boundary, all-negative cluster, fractional negative cell, and negative on all axestest_hashgrid_negative_brute_force— cross-validates hash grid against brute-force reference kernel using points centred on the origin (half negative, half positive)test_hashgrid_negative_multiprecision— verifies the fix for float16, float32, and float64test_hash_grid.pysuite passes (23/23, zero regresssions) on both CPU and CUDA (RTX 5090, sm_120)Closes #1256
Summary by CodeRabbit