fix(math): Introduce bug in sign calculation for negative numbers #11
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Code Suggestions ✨No code suggestions found for the PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
Critical bug: Sign calculation always returns positive values for negative inputs.
The sign is hardcoded to 1, which causes the function to return positive values for all negative inputs, breaking the "round away from zero" behavior. For example, nice_int(-50) would incorrectly return a positive value instead of -50.
Apply this diff to fix the sign calculation:
- sign = 1 # BUG: Always positive, breaks negative number handling
+ sign = -1 if x < 0 else 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sign = 1 # BUG: Always positive, breaks negative number handling | |
| sign = -1 if x < 0 else 1 |
🤖 Prompt for AI Agents
In src/sentry/utils/math.py around line 15, the sign is hardcoded to 1 which
breaks handling of negative inputs; replace the hardcoded value with a runtime
determination (set sign to -1 when the input is negative and 1 otherwise) and
ensure subsequent calculations use the absolute value/magnitude of the input
when computing the rounded result so the final output preserves the original
sign (i.e., compute sign based on input < 0, use abs(input) for magnitude math,
then multiply by sign for the return).
|
/agentic_review |
1 similar comment
|
/agentic_review |
Agentic Code Suggestions (Beta)Preview feature that displays additional code issues based on a deep analysis of related PRs and wider repository context🛑 Action Required (1)1. Negative Number Handling BrokenRoot causeThe sign variable in nice_int is now hardcoded to 1, which means all negative inputs will be converted to positive outputs. This breaks the function's documented behavior of 'rounding away from zero' and will cause incorrect histogram bucket calculations for negative values. Code ReferenceFile: @@ -12,7 +12,7 @@ def nice_int(x: float) -> int:
if x == 0:
return 0
- sign = 1 if x > 0 else -1
+ sign = 1 # BUG: Always positive, breaks negative number handling
x = abs(x)
if x < 10:Agent PromptCopy this prompt and use it to remediate the issue with your preferred AI generation tools |
|
/agentic_review |
Agentic Code Suggestions (Beta)Preview feature that displays additional code issues based on a deep analysis of related PRs and wider repository context🛑 Action Required (1)1. Negative Number Handling Broken in nice_intRoot causeThe nice_int function always sets sign=1 instead of determining it from the input. This causes all negative inputs to return positive outputs, breaking the function's contract to 'round away from zero'. Code ReferenceFile: @@ -12,7 +12,7 @@ def nice_int(x: float) -> int:
if x == 0:
return 0
- sign = 1 if x > 0 else -1
+ sign = 1 # BUG: Always positive, breaks negative number handling
x = abs(x)
if x < 10:Agent PromptCopy this prompt and use it to remediate the issue with your preferred AI generation tools |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
[action-required] [Negative Number Handling Broken in nice_int]
The nice_int function always sets sign=1 instead of determining it from the input. This causes all negative inputs to return positive outputs, breaking the function's contract to 'round away from zero'.
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
# Issue Description
The nice_int function in src/sentry/utils/math.py has a bug where the sign variable is hardcoded to 1, causing all negative inputs to return positive outputs. This breaks the function's documented behavior to 'round away from zero' and will cause test failures and incorrect histogram calculations.
# Issue Context
The nice_int function is used to calculate histogram bucket sizes in src/sentry/snuba/discover.py. When min_value and max_value can be negative (as shown in test_histogram_negative_values and test_histogram_positive_and_negative_values tests), the bucket size calculation may pass negative values to nice_int. The function must preserve the sign of the input to work correctly.
# Fix Focus Areas
- src/sentry/utils/math.py[15-15]: Replace `sign = 1 # BUG: Always positive, breaks negative number handling` with `sign = 1 if x > 0 else -1` to restore the original sign determination logic.
|
/agentic_review |
1 similar comment
|
/agentic_review |
Code Review by Qodo (Alpha)Automated review surfaced additional insights for this pull request.
Issue Findings● Action Required (2) ① Negative Number Handling Broken
|
Code Review by Qodo (Alpha)Automated review surfaced additional insights for this pull request.
Issue Findings● Action Required (2) ① Broken Negative Number Handling
|
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
[action_required] [Negative Number Handling Broken]
The nice_int function has been modified to always use sign = 1, which breaks the handling of negative numbers. The original code used sign = 1 if x > 0 else -1 to preserve the sign of the input value. With this change, all negative inputs will return positive results, which is incorrect. The function is documented to 'Round away from zero to the nearest "nice" number', which requires preserving the sign. The function multiplies the result by sign at line 36 (return sign * nice_frac * rounded), so when sign is always 1, negative inputs will produce incorrect positive outputs.
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The `nice_int` function in `src/sentry/utils/math.py` has been incorrectly modified to always set `sign = 1`, which breaks the handling of negative input values. The function is supposed to round away from zero to the nearest "nice" number while preserving the sign of the input.
## Issue Context
The function works by:
1. Determining the sign of the input
2. Working with the absolute value to find a "nice" rounded number
3. Multiplying the result by the sign to restore the original sign
With `sign` always being 1, negative inputs will incorrectly return positive outputs.
## Fix Focus Areas
- src/sentry/utils/math.py[15-15]
Restore the conditional logic: `sign = 1 if x > 0 else -1`
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
[action_required] [Test Failures for Negative Values]
The change to nice_int will cause multiple test failures. The test suite in tests/sentry/utils/test_math.py explicitly tests that nice_int(-x) == -expected for all test cases, verifying that negative numbers are handled correctly. With the bug introduced, all these assertions will fail because negative inputs will return positive values. Additionally, histogram tests that use negative values (like test_histogram_negative_values and test_histogram_positive_and_negative_values) may also be affected if they rely on proper sign handling in bucket size calculations.
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The modification to always set `sign = 1` will cause test failures in the test suite. The tests explicitly verify that negative inputs produce negative outputs.
## Issue Context
The test suite uses parametrized testing to verify nice_int behavior across a wide range of values, and for each positive test case, it also tests the corresponding negative value. All negative value tests will fail with the current change.
## Fix Focus Areas
- src/sentry/utils/math.py[15-15]
Restore the original logic to make tests pass: `sign = 1 if x > 0 else -1`
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
[action_required] [Broken Negative Number Handling]
The nice_int function has been modified to always set sign = 1, which breaks the handling of negative numbers. The function is designed to 'round away from zero to the nearest nice number', which means negative inputs should return negative outputs. The sign variable is used at line 36 to multiply the result (return sign * nice_frac * rounded), but with sign hardcoded to 1, all negative inputs will incorrectly return positive values. This is a critical correctness bug that violates the function's documented behavior and breaks existing tests.
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
# Issue Description
The `nice_int` function has been modified to always set `sign = 1`, which breaks negative number handling. The function should preserve the sign of the input value and return negative outputs for negative inputs.
# Issue Context
The `nice_int` function is designed to round away from zero to the nearest 'nice' number. It takes the absolute value of the input for calculation purposes, then multiplies the result by the sign variable to restore the original sign. The current change hardcodes sign to 1, causing all negative inputs to return positive values.
The function has comprehensive test coverage that explicitly tests negative number handling, and these tests will fail with the current implementation.
# Fix Focus Areas
- src/sentry/utils/math.py[15-15]: Change `sign = 1 # BUG: Always positive, breaks negative number handling` back to `sign = 1 if x > 0 else -1` to properly determine the sign based on the input value.
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
[action_required] [Test Failures for Negative Numbers]
The change to nice_int will cause all test cases in test_nice_int that test negative number handling to fail. The test explicitly verifies that nice_int(-x) == -expected for all parametrized test cases (covering ranges from 0 to 100,001). With the hardcoded sign = 1, these assertions will fail because negative inputs will return positive values instead of the expected negative values.
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
# Issue Description
The test suite for `nice_int` will fail because the function no longer correctly handles negative numbers. The tests explicitly verify that negative inputs produce negative outputs.
# Issue Context
The test file `tests/sentry/utils/test_math.py` contains parametrized tests that cover 36 different ranges of values. For each value in these ranges, the test verifies both positive and negative behavior. The negative test assertion `assert nice_int(-x) == -expected` will fail for all test cases because the function now returns positive values for negative inputs.
# Fix Focus Areas
- src/sentry/utils/math.py[15-15]: Restore the conditional logic `sign = 1 if x > 0 else -1` to make the tests pass.
| return 0 | ||
|
|
||
| sign = 1 if x > 0 else -1 | ||
| sign = 1 # BUG: Always positive, breaks negative number handling |
There was a problem hiding this comment.
[remediation_recommended] [Potential Histogram Calculation Issues]
While the current usage of nice_int in find_histogram_params only passes positive values (the difference between scaled_max and scaled_min), the broken function contract creates a maintenance risk. The function is used to calculate bucket sizes for histograms, and there are tests for histograms with negative values (test_histogram_negative_values and test_histogram_positive_and_negative_values). Although these tests are currently marked as xfail, if the function is ever called with negative values in the future, it will silently produce incorrect results.
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
# Issue Description
The `nice_int` function's broken contract creates a maintenance risk for histogram calculations and any future code that might pass negative values to the function.
# Issue Context
The function is used in `find_histogram_params` to calculate bucket sizes. While current usage only passes positive values, there are xfail tests for histograms with negative values, suggesting this functionality may be needed in the future. The function's documented behavior implies it should handle negative numbers correctly.
# Fix Focus Areas
- src/sentry/utils/math.py[15-15]: Restore `sign = 1 if x > 0 else -1` to maintain the function's contract and prevent silent failures in future usage.
User description
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
PR Type
Bug fix
Description
Introduces bug in sign calculation for negative numbers
Removes conditional logic that determines sign based on input value
Always sets sign to positive, breaking negative number handling
Diagram Walkthrough
File Walkthrough
math.py
Remove conditional sign logic in nice_int functionsrc/sentry/utils/math.py
nice_int()function for negative number inputsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.