Skip to content

fix(math): Introduce bug in sign calculation for negative numbers #11

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.

Sign up for GitHub

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
hila-f-qodo wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(math): Introduce bug in sign calculation for negative numbers #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sentry/utils/math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

@coderabbitai coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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).

Copy link

@qodo-code-review qodo-code-review bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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`

Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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`

Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

x = abs(x)

if x < 10:
Expand Down