Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 13, 2026

Summary by CodeRabbit

  • Tests

    • Expanded integer operation tests with more edge and boundary values
    • Increased floating-point test dataset with many additional edge cases
  • Improvements

    • Improved numerical stability and overflow handling in complex math operations
    • Adjusted complex magnitude and polar calculations for more robust behavior
  • Chores

    • Updated project ignore rules (added "cpython")

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Walkthrough

Expanded test edge-case datasets for integers and floats, adjusted complex-math magnitude and logarithm computations for numeric stability, and added "cpython" to .gitignore. No public APIs were changed.

Changes

Cohort / File(s) Summary
Configuration
\.gitignore
Added cpython to ignore rules.
Test data — integers & floats
src/math/integer.rs, src/test.rs
Enlarged EDGE_I64 from a fixed-size array to a slice and added many boundary values; expanded EDGE_VALUES from 30 to 64 f64 entries (subnormals, signaling NaN, trig-related and larger magnitudes). Adjusted test iteration accordingly.
Complex math — exponential
src/cmath/exponential.rs
Introduced mul_add import and refactored ln(z) path to precompute a log1p_arg using mul_add, then use log1p/2.0 for near-unit magnitudes.
Complex math — magnitude helpers
src/cmath/misc.rs
Added private helpers c_abs_raw and c_abs_checked; abs now uses c_abs_raw, and polar uses c_abs_checked, centralizing finite checks and overflow handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nibble at edges, keen and neat,

Added numbers, tests, and a tiny treat.
cpython hides, magnitudes refined,
Logs tamed with mul_add, neatly aligned.
Hooray — more signals for this curious mind!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'more edge values' directly aligns with the primary changes: expanding edge case test datasets in EDGE_I64, EDGE_VALUES, and adjusting test iteration patterns.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
src/cmath/misc.rs (1)

47-59: Consider removing the unnecessary set_errno(0) call.

The overflow detection relies on r.is_infinite() rather than checking errno, making the set_errno(0) on line 52 unnecessary. While valid per coding guidelines, it adds overhead without benefit.

♻️ Suggested simplification
 #[inline]
 fn c_abs_checked(z: Complex64) -> Result<f64> {
     if !z.re.is_finite() || !z.im.is_finite() {
         return Ok(c_abs_raw(z));
     }
-    crate::err::set_errno(0);
     let r = m::hypot(z.re, z.im);
     if r.is_infinite() {
         Err(Error::ERANGE)
     } else {
         Ok(r)
     }
 }

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8313a32 and 0c83ff1.

📒 Files selected for processing (3)
  • src/cmath/exponential.rs
  • src/cmath/misc.rs
  • src/math/integer.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Error handling: Return Result directly with Err(crate::Error::EDOM) or Err(crate::Error::ERANGE) instead of using set_errno. Only valid set_errno call is set_errno(0) to clear errno before libm calls.

Files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
  • src/math/integer.rs
src/cmath/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/cmath/**/*.rs: For complex math (cmath module), copy special_type() enum and function, 7x7 special value tables (e.g., tanh_special_values), SPECIAL_VALUE macro usage, and complex-specific error handling from Modules/cmathmodule.c.
Use mul_add in cmath functions for expressions like 1.0 + x * x, cross-product calculations (mul_add(s1.re, s2.im, -(s2.re * s1.im))), and squared terms to achieve bit-exact matching with CPython.

Files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
src/math/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/math/**/*.rs: Use existing CPython helper equivalents: FUNC1 → math_1(x, func, can_overflow), FUNC2 → math_2(x, y, func), m_log/m_log10/m_log2 in exponential.rs. If CPython uses a helper, use the corresponding Rust helper.
Use crate::mul_add(a, b, c) instead of a * b + c for expressions of the form a * b + c or c + a * b to match CPython's FMA (fused multiply-add) behavior on platforms like macOS where it's enabled.

Files:

  • src/math/integer.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/cmath/**/*.rs : Use mul_add in cmath functions for expressions like 1.0 + x * x, cross-product calculations (mul_add(s1.re, s2.im, -(s2.re * s1.im))), and squared terms to achieve bit-exact matching with CPython.
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/**/*test*.rs : All float functions must be tested with crate::test::EDGE_VALUES which includes zeros, infinities, NaNs with different payloads, subnormals, boundary values (MIN_POSITIVE, MAX, MIN), large values near infinity, and trigonometric special values (PI, PI/2, PI/4, TAU).
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/cmath/**/*.rs : For complex math (cmath module), copy special_type() enum and function, 7x7 special value tables (e.g., tanh_special_values), SPECIAL_VALUE macro usage, and complex-specific error handling from Modules/cmathmodule.c.
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/cmath/**/*.rs : For complex math (cmath module), copy special_type() enum and function, 7x7 special value tables (e.g., tanh_special_values), SPECIAL_VALUE macro usage, and complex-specific error handling from Modules/cmathmodule.c.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/cmath/**/*.rs : Use mul_add in cmath functions for expressions like 1.0 + x * x, cross-product calculations (mul_add(s1.re, s2.im, -(s2.re * s1.im))), and squared terms to achieve bit-exact matching with CPython.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/cmath/trigonometric.rs : In cmath functions that use both sin and cos of the same angle (cosh, sinh, exp, rect), use m::sincos(x) which returns (sin, cos) tuple instead of calling sin and cos separately to match CPython's macOS __sincos_stret behavior.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/math/**/*.rs : Use existing CPython helper equivalents: FUNC1 → math_1(x, func, can_overflow), FUNC2 → math_2(x, y, func), m_log/m_log10/m_log2 in exponential.rs. If CPython uses a helper, use the corresponding Rust helper.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/math/**/*.rs : Use crate::mul_add(a, b, c) instead of a * b + c for expressions of the form a * b + c or c + a * b to match CPython's FMA (fused multiply-add) behavior on platforms like macOS where it's enabled.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/math.rs : Create missing helpers if CPython has a helper we don't have yet, such as math_1_fn for Rust function pointers or special case handlers for specific functions.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/math/exponential.rs : Copy CPython's explicit special case handling for IEEE specials (NaN, Inf, etc.) exactly, including logic for cases like NaN**0 = 1.

Applied to files:

  • src/cmath/misc.rs
  • src/cmath/exponential.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/**/*.rs : Error handling: Return Result directly with Err(crate::Error::EDOM) or Err(crate::Error::ERANGE) instead of using set_errno. Only valid set_errno call is set_errno(0) to clear errno before libm calls.

Applied to files:

  • src/cmath/misc.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/**/*test*.rs : All float functions must be tested with crate::test::EDGE_VALUES which includes zeros, infinities, NaNs with different payloads, subnormals, boundary values (MIN_POSITIVE, MAX, MIN), large values near infinity, and trigonometric special values (PI, PI/2, PI/4, TAU).

Applied to files:

  • src/cmath/misc.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.994Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.994Z
Learning: Applies to src/**/*test*.rs : Tests must verify both correct values for Ok results and correct error types (EDOM vs ERANGE) for Err results. Python ValueError maps to Error::EDOM and OverflowError maps to Error::ERANGE.

Applied to files:

  • src/math/integer.rs
🧬 Code graph analysis (2)
src/cmath/misc.rs (3)
src/math/misc.rs (1)
  • fabs (30-32)
src/math.rs (1)
  • hypot (109-130)
src/err.rs (1)
  • set_errno (30-58)
src/cmath/exponential.rs (2)
src/lib.rs (1)
  • mul_add (27-33)
src/math/exponential.rs (1)
  • log1p (156-158)
🔇 Additional comments (7)
src/cmath/exponential.rs (2)

7-7: LGTM!

Import of mul_add aligns with the coding guidelines for achieving bit-exact matching with CPython.


163-164: Good use of mul_add for numeric stability.

The refactored computation correctly applies FMA to the (am - 1.0) * (am + 1.0) + an² expression, which improves numeric stability for complex logarithms near unit magnitude. This aligns with the coding guidelines for achieving bit-exact matching with CPython.

src/cmath/misc.rs (3)

31-45: LGTM!

The c_abs_raw helper correctly implements C99 Annex G semantics: infinity takes precedence over NaN when computing complex magnitude. The check order (infinity before NaN) is correct.


63-66: LGTM!

The polar() function correctly uses c_abs_checked() to propagate overflow errors. The refactoring cleanly separates the error-handling path from the raw computation.


122-125: LGTM!

The abs() function appropriately delegates to c_abs_raw() since it returns f64 (not Result) and follows C99 semantics for special values.

src/math/integer.rs (2)

718-773: Well-documented edge value expansion.

The expanded EDGE_I64 dataset provides comprehensive coverage of algorithm boundaries, including:

  • Comb/perm table boundaries (127, 128)
  • Factorial overflow thresholds (170, 171)
  • Fast-path switching points (34, 35)
  • Square root computation boundaries

The inline comments explaining each value's significance are helpful for maintainability. Based on learnings, this aligns with the testing requirement for boundary values.


985-990: LGTM!

The iteration syntax correctly adapts to the slice-based EDGE_I64. Using for &a in EDGE_I64 directly iterates over the slice and destructures to get i64 values.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/math/integer.rs (1)

718-773: Fix array size mismatch causing compilation failure.

The pipeline failure indicates the array declaration specifies 44 elements but 45 are provided. Update the type to [i64; 45] to match the actual element count.

🐛 Proposed fix
-    const EDGE_I64: [i64; 44] = [
+    const EDGE_I64: [i64; 45] = [
🧹 Nitpick comments (1)
src/test.rs (1)

61-65: Duplicate value in array.

-0.9999999999999999 appears twice: once at line 62 (atanh boundary) and again at line 64 (log1p boundary). Consider replacing one with a distinct value or removing the duplicate to maximize test coverage with unique values.

♻️ Suggested fix - replace duplicate with a distinct value
     // log1p domain boundary (> -1)
-    -0.9999999999999999, // just above -1
+    -0.99999999999999, // slightly further from -1
     -1.0 + 1e-15,        // very close to -1
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 97d2f52 and 8313a32.

📒 Files selected for processing (3)
  • .gitignore
  • src/math/integer.rs
  • src/test.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Error handling: Return Result directly with Err(crate::Error::EDOM) or Err(crate::Error::ERANGE) instead of using set_errno. Only valid set_errno call is set_errno(0) to clear errno before libm calls.

Files:

  • src/test.rs
  • src/math/integer.rs
src/**/*test*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*test*.rs: All float functions must be tested with crate::test::EDGE_VALUES which includes zeros, infinities, NaNs with different payloads, subnormals, boundary values (MIN_POSITIVE, MAX, MIN), large values near infinity, and trigonometric special values (PI, PI/2, PI/4, TAU).
Tests must verify both correct values for Ok results and correct error types (EDOM vs ERANGE) for Err results. Python ValueError maps to Error::EDOM and OverflowError maps to Error::ERANGE.

Files:

  • src/test.rs
src/math/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/math/**/*.rs: Use existing CPython helper equivalents: FUNC1 → math_1(x, func, can_overflow), FUNC2 → math_2(x, y, func), m_log/m_log10/m_log2 in exponential.rs. If CPython uses a helper, use the corresponding Rust helper.
Use crate::mul_add(a, b, c) instead of a * b + c for expressions of the form a * b + c or c + a * b to match CPython's FMA (fused multiply-add) behavior on platforms like macOS where it's enabled.

Files:

  • src/math/integer.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/**/*test*.rs : All float functions must be tested with crate::test::EDGE_VALUES which includes zeros, infinities, NaNs with different payloads, subnormals, boundary values (MIN_POSITIVE, MAX, MIN), large values near infinity, and trigonometric special values (PI, PI/2, PI/4, TAU).
📚 Learning: 2026-01-10T13:40:01.981Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/**/*test*.rs : All float functions must be tested with crate::test::EDGE_VALUES which includes zeros, infinities, NaNs with different payloads, subnormals, boundary values (MIN_POSITIVE, MAX, MIN), large values near infinity, and trigonometric special values (PI, PI/2, PI/4, TAU).

Applied to files:

  • src/test.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.981Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/**/*test*.rs : Tests must verify both correct values for Ok results and correct error types (EDOM vs ERANGE) for Err results. Python ValueError maps to Error::EDOM and OverflowError maps to Error::ERANGE.

Applied to files:

  • src/test.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.981Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/**/*.rs : Error handling: Return Result directly with Err(crate::Error::EDOM) or Err(crate::Error::ERANGE) instead of using set_errno. Only valid set_errno call is set_errno(0) to clear errno before libm calls.

Applied to files:

  • src/test.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.981Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/math/exponential.rs : Copy CPython's explicit special case handling for IEEE specials (NaN, Inf, etc.) exactly, including logic for cases like NaN**0 = 1.

Applied to files:

  • src/test.rs
📚 Learning: 2026-01-10T13:40:01.981Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/math.rs : Create missing helpers if CPython has a helper we don't have yet, such as math_1_fn for Rust function pointers or special case handlers for specific functions.

Applied to files:

  • src/test.rs
  • src/math/integer.rs
📚 Learning: 2026-01-10T13:40:01.981Z
Learnt from: CR
Repo: RustPython/pymath PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T13:40:01.981Z
Learning: Applies to src/math/**/*.rs : Use existing CPython helper equivalents: FUNC1 → math_1(x, func, can_overflow), FUNC2 → math_2(x, y, func), m_log/m_log10/m_log2 in exponential.rs. If CPython uses a helper, use the corresponding Rust helper.

Applied to files:

  • src/math/integer.rs
🪛 GitHub Actions: Rust
src/math/integer.rs

[error] 718-718: mismatched types: expected an array with a size of 44, found one with a size of 45 for EDGE_I64. Please adjust the array length to 44 to match the actual number of elements.

🔇 Additional comments (3)
.gitignore (1)

3-3: LGTM!

Adding cpython to .gitignore is appropriate for ignoring a local CPython checkout used for testing purposes.

src/math/integer.rs (1)

719-773: Good expansion of edge case coverage.

The expanded EDGE_I64 values provide comprehensive coverage including:

  • Table boundaries (127/128 for comb/perm lookup tables)
  • Algorithm switching points (34/35 for FAST_COMB_LIMITS1)
  • Square root boundaries for different code paths
  • Factorial overflow thresholds

The test implementations properly verify both Ok values and error types (EDOM for negative inputs). Based on learnings, this aligns with the testing guidelines.

src/test.rs (1)

6-91: Comprehensive edge value coverage.

The expanded EDGE_VALUES array now provides excellent coverage as per coding guidelines:

  • ✓ Zeros (positive and negative)
  • ✓ Infinities
  • ✓ NaNs with different payloads (including signaling NaN)
  • ✓ Subnormals
  • ✓ Boundary values (MIN_POSITIVE, MAX, MIN)
  • ✓ Large values near infinity
  • ✓ Trigonometric special values (PI, PI/2, PI/4, TAU)
  • ✓ Domain boundaries for asin/acos, atanh, log1p, gamma

The signaling NaN inclusion with the platform-specific exception warning is appropriately documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants