-
Notifications
You must be signed in to change notification settings - Fork 504
reduced precision ball query #1266
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
base: main
Are you sure you want to change the base?
reduced precision ball query #1266
Conversation
…parently casting and then casting back.
|
/blossom-ci |
Greptile OverviewGreptile SummaryImplements reduced precision support for radius search/ball query operations by adding transparent casting to float32 and back. Since Warp's hashmap doesn't natively support half precision, the implementation casts inputs to float32 before processing and converts results back to the original precision. Key changes:
The implementation is straightforward and correct. Output points and distances are properly converted back to match the input precision. Important Files ChangedFile Analysis
|
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.
Additional Comments (2)
-
physicsnemo/utils/neighbors/radius_search/_warp_impl.py, line 367-371 (link)style: Variable shadowing -
pointsparameter is overwritten here, which could cause confusion and breaks the reference to the input tensor for later casting back.When
return_points=Trueandmax_pointsis not None, this reassigns thepointsvariable (line 367-371), shadowing the input parameter. Later on line 403,points.to(input_dtype)attempts to cast, but it's operating on the newly created tensor, not the original input. While this works functionally (the new tensor is created withdtype=torch.float32), it's confusing because the variable name collision makes the code harder to follow. -
physicsnemo/utils/neighbors/radius_search/_warp_impl.py, line 332 (link)style: Same variable shadowing issue for
pointsin the unlimited neighbors path.The
gather_neighborsfunction returns a new tensor namedpointswhich shadows the input parameterpoints. While functionally correct (sincegather_neighborscreates a float32 tensor that's later cast back on line 403), the variable reuse makes the code harder to understand.
2 files reviewed, 2 comments
peterdsharpe
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.
Overall this looks good. Bummer that Warp isn't dtype-flexible here, but assuming that's a given, I can't think of a better way to handle this situation.
Before merging, can we add a quick note in the docstring (either in _warp_impl or in the top-level wrapper, at your choice) indicating that this does an internal cast to fp32 (and hence, a new allocation) of the associated arrays?
peterdsharpe
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.
One last thing - let's check fp32 in tests
Co-authored-by: Peter Sharpe <[email protected]>
|
/blossom-ci |
|
Thanks for the reviews! I added a note about the transparent casting, I'll merge when CI is happy. |
|
/blossom-ci |
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.
3 files reviewed, no comments
Add reduced precision support for radius search / ball query by transparently casting and then casting back.
Warp hashmap doesn't actually support half precision, so this is our only path to functionality at this time.
PhysicsNeMo Pull Request
Description
Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.