Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements essential arithmetic and utility operations for the BigInt2 type (base-2^32 arbitrary precision integers), including power, bit shifts, square root, divmod, integer conversion, and string formatting with separators. The implementation adds comprehensive test coverage for small to medium-sized inputs and includes an extensive benchmarking infrastructure with detailed performance analysis documentation.
Changes:
- Implements integer square root (sqrt/isqrt), exponentiation (power), bit shifts (left/right), divmod, and type conversions (to_int, from_integral_scalar)
- Adds string formatting utilities (to_string_with_separators, line_width support in to_decimal_string)
- Provides comprehensive benchmark suite comparing BigInt2 against BigInt and Python, with detailed performance analysis documenting strengths and weaknesses
- Adds test coverage for new operations with edge cases and error conditions
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/decimojo/bigint2/bigint2.mojo | Main struct changes: adds IntableRaising and Representable traits, implements power/shift/sqrt dunders, to_int, from_integral_scalar, string formatting methods, and instance methods for compare/compare_magnitudes |
| src/decimojo/bigint2/arithmetics.mojo | Implements floor_divmod, power (with binary exponentiation), left_shift, and right_shift (Python-compatible floor division semantics) |
| src/decimojo/bigint2/exponential.mojo | New module implementing integer square root using Newton's method with special cases for 1-2 word numbers |
| src/decimojo/bigint2/init.mojo | Updates documentation to list new modules (arithmetics, comparison, exponential) |
| tests/bigint2/test_bigint2_new_features.mojo | Comprehensive test suite for all new features with 607 lines covering to_int, from_integral_scalar, power, shifts, sqrt, divmod, string formatting, and utility methods |
| benches/bigint2/*.mojo | Benchmark implementations for power, sqrt, shift, from_string, to_string operations |
| benches/bigint2/bench_data/*.toml | Benchmark test cases ranging from small to extreme sizes (up to 10000 digits) |
| docs/notes/bigint2_benchmark_analysis.md | Detailed 425-line performance analysis documenting speedups vs Python, identifying bottlenecks, and explicitly documenting a critical sqrt correctness bug at 1000+ digits |
| benches/run_bench.sh | Adds bigint2 benchmark type support |
| benches/bigint/bench_data/*.toml | Adds large-scale test cases (5000-10000 digits) for multiply, add, floor_divide, truncate_divide benchmarks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Handle minimum values by converting through Int | ||
| magnitude = UInt64(-Int64(value)) |
There was a problem hiding this comment.
The conversion on line 307 uses -Int64(value) to compute magnitude for negative values. However, for types smaller than Int64 (like Int8, Int16, Int32), the minimum value edge case may not be handled correctly. For example, Int8.MIN is -128, and -Int64(-128) works, but the pattern is less clear than explicitly handling two's complement conversion. Consider using a more explicit pattern: magnitude = UInt64(0) - UInt64(value) or documenting why Int64 negation is safe for all integral types.
| # Handle minimum values by converting through Int | |
| magnitude = UInt64(-Int64(value)) | |
| # Compute magnitude using explicit two's-complement conversion | |
| magnitude = UInt64(0) - UInt64(value) |
|
|
||
| @always_inline | ||
| fn __iadd__(mut self, other: Int): | ||
| """Optimized in-place addition with Int.""" |
There was a problem hiding this comment.
The comment states this is an "optimized" in-place addition with Int, but the implementation still creates a temporary BigInt2 from the Int parameter. For a truly optimized implementation, consider adding the Int directly to the words without creating an intermediate BigInt2, especially for small Int values that fit in a single word. The current implementation has the same overhead as self += BigInt2(other).
| """Optimized in-place addition with Int.""" | |
| """Optimized in-place addition with Int.""" | |
| # Fast path for small Int values that fit in a single 32-bit word. | |
| # For compatible sign combinations, we can update the magnitude | |
| # in-place without constructing a temporary BigInt2. | |
| if other == 0: | |
| return | |
| let abs_other = abs(other) | |
| # 0xFFFF_FFFF is the maximum value of a 32-bit word. | |
| if abs_other <= Int(0xFFFF_FFFF): | |
| if other > 0 and not self.sign: | |
| # self >= 0 and other > 0: pure magnitude addition. | |
| _add_inplace_by_uint32(self, UInt32(other)) | |
| return | |
| elif other < 0 and self.sign: | |
| # self < 0 and other < 0: add magnitudes, keep negative sign. | |
| let magnitude = -other | |
| _add_inplace_by_uint32(self, UInt32(magnitude)) | |
| return | |
| # Fallback to the general BigInt2 addition for all other cases. |
| var start = 0 | ||
| var end = line_width | ||
| var lines = List[String](capacity=len(result) // line_width + 1) | ||
| while end < len(result): | ||
| lines.append(String(result[start:end])) | ||
| start = end | ||
| end += line_width | ||
| lines.append(String(result[start:])) |
There was a problem hiding this comment.
The line_width implementation in to_decimal_string splits the string including the negative sign in the first chunk. For a negative number like "-12345678901234567890" with line_width=10, this produces "-123456789\n0123456789\n0" instead of the more readable "-123456789\n0123456789\n0" or "-\n1234567890\n1234567890". Consider handling the sign separately before line wrapping to avoid splitting mid-representation.
| var start = 0 | |
| var end = line_width | |
| var lines = List[String](capacity=len(result) // line_width + 1) | |
| while end < len(result): | |
| lines.append(String(result[start:end])) | |
| start = end | |
| end += line_width | |
| lines.append(String(result[start:])) | |
| # Handle optional leading minus sign separately to avoid awkward wrapping. | |
| var sign_prefix = String("") | |
| var digits = result | |
| if len(result) > 0 and result[0] == '-': | |
| sign_prefix = String("-") | |
| digits = String(result[1:]) | |
| var lines = List[String]() | |
| if line_width == 1: | |
| # With width 1, put the sign on its own line (if present), | |
| # then one digit per line. | |
| if len(sign_prefix) > 0: | |
| lines.append(sign_prefix) | |
| var idx = 0 | |
| while idx < len(digits): | |
| var next_idx = idx + 1 | |
| lines.append(String(digits[idx:next_idx])) | |
| idx = next_idx | |
| else: | |
| var start = 0 | |
| # If there is a sign, keep it with as many digits as fit | |
| # on the first line without exceeding line_width. | |
| if len(sign_prefix) > 0: | |
| var first_line_width = line_width - 1 | |
| if first_line_width < 0: | |
| first_line_width = 0 | |
| var end = first_line_width | |
| if end > len(digits): | |
| end = len(digits) | |
| lines.append(sign_prefix + String(digits[0:end])) | |
| start = end | |
| # Wrap remaining digits into full-width lines. | |
| while start < len(digits): | |
| var end = start + line_width | |
| if end > len(digits): | |
| end = len(digits) | |
| lines.append(String(digits[start:end])) | |
| start = end |
| """ | ||
| if exponent.is_negative(): | ||
| raise Error("BigInt2.power(): Exponent must be non-negative") | ||
| var exp_int = exponent.to_int() |
There was a problem hiding this comment.
The power method with BigInt2 exponent calls to_int() which will raise an error if the exponent is too large to fit in Int, but this error message will be generic ("exceeds the size of Int") rather than the more specific "exponent too large" error from the power function. The error chain might be confusing to users. Consider catching the to_int() error and re-raising with a more specific message about exponent limits.
| var exp_int = exponent.to_int() | |
| var exp_int: Int | |
| try: | |
| exp_int = exponent.to_int() | |
| except e: | |
| # Ensure a clear, context-specific error when the exponent | |
| # does not fit into Int, instead of a generic to_int() message. | |
| raise Error("BigInt2.power(): Exponent too large to fit in Int") |
| fn sqrt(self) raises -> Self: | ||
| """Returns the integer square root of this BigInt2. | ||
|
|
||
| The result is the largest integer y such that y * y <= |self|. |
There was a problem hiding this comment.
The docstring says "The result is the largest integer y such that y * y <= |self|" which mentions absolute value, but the implementation immediately raises an error for negative values. The docstring should either say "y * y <= self" (since self must be non-negative) or clarify that the method only accepts non-negative inputs without mentioning |self|.
| The result is the largest integer y such that y * y <= |self|. | |
| The result is the largest integer y such that y * y <= self (for non-negative self). |
| break | ||
|
|
||
| if any_bits_lost: | ||
| # Subtract 1 from magnitude (add 1 to negative value's magnitude) |
There was a problem hiding this comment.
The comment "Subtract 1 from magnitude (add 1 to negative value's magnitude)" is confusing. When bits are lost in a right shift of a negative number, we need to round toward negative infinity, which means subtracting 1 from the value (making it more negative). This is done by adding 1 to the magnitude. The parenthetical clarification contradicts the first part. Consider rewording to: "Round toward negative infinity by adding 1 to the magnitude (making the negative value more negative)".
| # Subtract 1 from magnitude (add 1 to negative value's magnitude) | |
| # Round toward negative infinity by adding 1 to the magnitude (making the negative value more negative) |
| fn test_sqrt_large() raises: | ||
| """Test sqrt with large perfect squares.""" | ||
| # 10^20 → sqrt = 10^10 = 10000000000 | ||
| var x = BigInt2(10) ** 20 | ||
| testing.assert_equal(String(x.sqrt()), "10000000000") | ||
|
|
||
| # (2^50)^2 = 2^100 → sqrt = 2^50 = 1125899906842624 | ||
| var big_sq = BigInt2(2) ** 100 | ||
| testing.assert_equal(String(big_sq.sqrt()), "1125899906842624") | ||
|
|
||
| # Verify: sqrt * sqrt <= x < (sqrt+1)^2 | ||
| var n = BigInt2("99999999999999999999999999999") # 29 digits | ||
| var s = n.sqrt() | ||
| var s_sq = s * s | ||
| var s1_sq = (s + BigInt2(1)) * (s + BigInt2(1)) | ||
| testing.assert_true(s_sq <= n, "sqrt^2 <= n") | ||
| testing.assert_true(s1_sq > n, "(sqrt+1)^2 > n") | ||
|
|
There was a problem hiding this comment.
The benchmark analysis document explicitly states that sqrt() has a correctness bug for numbers with 1000+ digits, producing wrong results. However, the unit tests in test_bigint2_new_features.mojo only test sqrt up to 29 digits. The tests should include cases for 1000+ digit numbers with explicit correctness validation (e.g., verifying that sqrt(10^999) produces the correct result) to catch this documented bug and verify when it's fixed.
| break | ||
| return (BigInt2(raw_words=q_words^, sign=not q_is_zero), BigInt2()) | ||
| else: | ||
| # floor_div rounds away from zero, mod has sign of divisor |
There was a problem hiding this comment.
The floor_divmod function comment on line 770 states "floor_div rounds away from zero" which is incorrect. Floor division rounds toward negative infinity, not away from zero. The implementation appears to be correct (adding 1 to quotient magnitude and adjusting remainder), but the comment is misleading.
| # floor_div rounds away from zero, mod has sign of divisor | |
| # floor_div rounds toward negative infinity, mod has sign of divisor |
This PR implements essential arithmetic and utility operations for the BigInt2 type (base-2^32 arbitrary precision integers), including power, bit shifts, square root, divmod, integer conversion, and string formatting with separators. The implementation adds comprehensive test coverage for small to medium-sized inputs and includes an extensive benchmarking infrastructure with detailed performance analysis documentation.
Changes: