-
Notifications
You must be signed in to change notification settings - Fork 473
Fix HashGrid truncation error with negative coordinates #1281
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
Open
cluster2600
wants to merge
4
commits into
NVIDIA:main
Choose a base branch
from
cluster2600:fix/hashgrid-negative-coord-truncation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+170
−33
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
94119ea
Fix HashGrid truncation error with negative coordinates
cluster2600 5578a4a
Apply clang-format and use RST backticks in docstrings
d269d08
Fix misleading comment and stale test registration
cluster2600 afdf46a
Merge branch 'main' into fix/hashgrid-negative-coord-truncation
cluster2600 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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_gridbuilds a grid vianp.linspace(0, dim, dim)(range[0, 8]) scaled byradius * 2.0 = 1.0, then shifted bylower = (-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: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the comment wording — you're right that with
lower=(-8, -8, -8)andlinspace(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot — updated the lower bound to (-4, -4, -4) so the points actually straddle the origin as the comment says. Cheers.