Conversation
There was a problem hiding this comment.
Pull request overview
Updates the CLI calculator’s evaluation pipeline to use a higher “working precision” internally (guard digits) and then round the final displayed result to the requested precision, with a new CLI option to choose the final rounding mode.
Changes:
- Switch intermediate evaluation to
working_precision = precision + GUARD_DIGITSand addfinal_round(...)for final significant-digit rounding. - Add
--rounding-modeto the CLI and apply it when producing the final output. - Update the showcase evaluator test expected output to reflect significant-digit rounding; add
.ruff_cacheto.gitignore.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/cli/test_evaluator.mojo | Adjusts expected output for the showcase expression under significant-digit semantics. |
| src/cli/main.mojo | Adds --rounding-mode, updates precision help text, and rounds final results before printing. |
| src/cli/calculator/evaluator.mojo | Implements working-precision evaluation, trims multiplication intermediates, and adds final_round + rounding-mode support. |
| .gitignore | Ignores Ruff cache directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -58,7 +59,7 @@ fn _run() raises: | |||
|
|
|||
| # Named option: decimal precision | |||
There was a problem hiding this comment.
The precision flag is now described as “number of significant digits”, but the surrounding comment still says “decimal precision”, which is misleading given the updated semantics. Please update the comment to match the new meaning (significant digits) so CLI docs stay consistent.
| # Named option: decimal precision | |
| # Named option: number of significant digits |
| Error: On division by zero, missing operands, or other runtime | ||
| errors — with source position when available. | ||
| """ | ||
| comptime GUARD_DIGITS = 9 # Word size |
There was a problem hiding this comment.
GUARD_DIGITS = 9 is a key accuracy/performance tradeoff but the comment “Word size” is unclear in this context (guard digits aren’t a word size unless you mean one base-1e9 BigUInt word ≈ 9 decimal digits). Please clarify the rationale (and ideally link it to the BigUInt base/word representation) so future changes don’t accidentally break precision guarantees.
| comptime GUARD_DIGITS = 9 # Word size | |
| # NOTE: BDec is backed by a BigUInt whose "word" base is 1e9, i.e., each | |
| # internal word represents up to ~9 decimal digits. We choose | |
| # GUARD_DIGITS = 9 so that `working_precision` has roughly one extra | |
| # BigUInt word of precision beyond the user‑requested `precision`. | |
| # This extra word acts as guard digits for intermediate operations | |
| # (pow, division, etc.) to reduce accumulated rounding error. | |
| # If the BigUInt base or digits‑per‑word change, this constant | |
| # should be revisited to maintain similar accuracy/performance | |
| # characteristics. | |
| comptime GUARD_DIGITS = 9 |
| fn final_round( | ||
| value: BDec, | ||
| precision: Int, | ||
| rounding_mode: RoundingMode = RoundingMode.half_even(), | ||
| ) raises -> BDec: | ||
| """Round a BigDecimal to `precision` significant digits. | ||
|
|
||
| This should be called on the result of `evaluate_rpn` before | ||
| displaying it to the user, so that guard digits are removed and | ||
| the last visible digit is correctly rounded. | ||
| """ | ||
| if value.is_zero(): | ||
| return value.copy() | ||
| var result = value.copy() | ||
| result.round_to_precision(precision, rounding_mode, False, False) | ||
| return result^ |
There was a problem hiding this comment.
final_round/evaluate(..., rounding_mode=...) adds new rounding-mode behavior, but there are no tests covering non-default rounding modes (especially tie cases where modes differ, e.g. half-even vs half-up). Please add a couple of evaluator tests that exercise evaluate(..., precision=..., rounding_mode=...) (or final_round) on values that hit a rounding tie to prevent regressions.
| # Named option: decimal precision | ||
| cmd.add_arg( | ||
| Arg("precision", help="Decimal precision for division (default: 50)") | ||
| Arg("precision", help="Number of significant digits (default: 50)") |
There was a problem hiding this comment.
The CLI now defines --precision as “number of significant digits”, but the --pad path still pads the fractional part to precision decimal places (via _pad_to_precision(..., precision)). This makes --pad inconsistent with the new precision semantics and can produce more than precision significant digits. Consider either implementing padding in significant-digits terms (e.g., using round_to_precision(..., fill_zeros_to_precision=True)), or introducing a separate --decimal-places/--scale flag for the current padding behavior.
| Arg("precision", help="Number of significant digits (default: 50)") | |
| Arg("precision", help="Number of decimal places (default: 50)") |
This PR updates the CLI calculator’s evaluation pipeline to use a higher “working precision” internally (guard digits) and then rounds the final displayed result to the requested precision, with a new CLI option to choose the final rounding mode. **Changes:** - Switch intermediate evaluation to `working_precision = precision + GUARD_DIGITS` and add `final_round(...)` for final significant-digit rounding. - Add `--rounding-mode` to the CLI and apply it when producing the final output. - Update the showcase evaluator test expected output to reflect significant-digit rounding; add `.ruff_cache` to `.gitignore`.
This PR updates the CLI calculator’s evaluation pipeline to use a higher “working precision” internally (guard digits) and then rounds the final displayed result to the requested precision, with a new CLI option to choose the final rounding mode.
Changes:
working_precision = precision + GUARD_DIGITSand addfinal_round(...)for final significant-digit rounding.--rounding-modeto the CLI and apply it when producing the final output..ruff_cacheto.gitignore.