Fixed the incorrect overflow check logic #1050
Conversation
Summary of ChangesHello @botbigeyes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug in the overflow checking mechanism, specifically for floating-point values. The previous implementation incorrectly flagged special floating-point values (infinity, negative infinity, and NaN) as overflows. The updated logic now correctly identifies and bypasses these special values before performing overflow checks, ensuring consistency with PyTorch's behavior. Additionally, a minor whitespace adjustment was made in an error message within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an incorrect overflow check for floating-point values by ignoring special values like inf and nan. While the intent is correct, the implementation introduces a critical regression where overflow checks are skipped for integer values being converted to float types. I've provided a comment with a suggested fix for this issue. The other changes in the PR are minor stylistic improvements.
src/flag_gems/ops/full.py
Outdated
| is_float = isinstance(fill_value, float) | ||
|
|
||
| if is_float and not (math.isinf(fill_value) or math.isnan(fill_value)): | ||
| if ( | ||
| fill_value < -torch.finfo(dtype).max | ||
| or fill_value > torch.finfo(dtype).max | ||
| ): | ||
| raise RuntimeError( | ||
| f"value cannot be converted to type {dtype} without overflow" | ||
| ) | ||
|
|
||
| fill_value = torch.tensor(fill_value, dtype=dtype, device=device) |
There was a problem hiding this comment.
The current logic for checking float overflow has a regression. By checking isinstance(fill_value, float), you are skipping the overflow check when fill_value is an integer. An integer can be large enough to overflow a floating-point type (e.g., fill_value=10**40 for dtype=torch.float32), which this new logic fails to catch. The previous implementation handled this correctly.
The check should only be skipped for special floating-point values like inf and nan. For all other numeric types, including integers, the overflow check should be performed.
Here is a suggested fix that correctly handles special float values while still checking for overflow from other numeric types. It also uses torch.finfo(dtype).min which is slightly more direct than -torch.finfo(dtype).max.
| is_float = isinstance(fill_value, float) | |
| if is_float and not (math.isinf(fill_value) or math.isnan(fill_value)): | |
| if ( | |
| fill_value < -torch.finfo(dtype).max | |
| or fill_value > torch.finfo(dtype).max | |
| ): | |
| raise RuntimeError( | |
| f"value cannot be converted to type {dtype} without overflow" | |
| ) | |
| fill_value = torch.tensor(fill_value, dtype=dtype, device=device) | |
| is_special_float = isinstance(fill_value, float) and (math.isinf(fill_value) or math.isnan(fill_value)) | |
| if not is_special_float: | |
| finfo = torch.finfo(dtype) | |
| if fill_value > finfo.max or fill_value < finfo.min: | |
| raise RuntimeError( | |
| f"value cannot be converted to type {dtype} without overflow" | |
| ) | |
| fill_value = torch.tensor(fill_value, dtype=dtype, device=device) |
src/flag_gems/ops/full.py
Outdated
| if is_float and not (math.isinf(fill_value) or math.isnan(fill_value)): | ||
| if ( | ||
| fill_value < -torch.finfo(dtype).max | ||
| or fill_value > torch.finfo(dtype).max | ||
| ): |
There was a problem hiding this comment.
Can we merge these two if statements?
There was a problem hiding this comment.
Yes, the structure can be merged and simplified.
|
plz merge master |
Ok, I have already merged the changes from the master branch into the full_like branch |
| raise RuntimeError( | ||
| f"value cannot be converted to type {dtype} without overflow" | ||
| ) | ||
| if dtype == torch.float64: |
There was a problem hiding this comment.
The del this if can unify the float conversion. As long as it's a float type, fill_value will be converted to the corresponding torch.tensor after passing the overflow check，I don't understand why it's necessary to handle float64 conversion separately.
3 participants
PR Category
Operator
Type of Change
Bug Fix
Description
The overflow check logic previously incorrectly considered special floating-point values (inf, -inf, nan), causing inconsistencies with PyTorch behavior. The logic has been fixed to first exclude special values before checking for overflow.
Issue
Progress
Performance