refactor: accumulate Number arithmetic symbolically#23
Closed
hozblok wants to merge 1 commit into
Closed
Conversation
The five arithmetic ops (+, -, *, /, ^) and the five reverse ops used to evaluate eagerly and store the formatted result string as the new Number's expression. Each subsequent op re-parsed that rounded string, so chained operations on high-precision Numbers could no longer be better than a series of round-trips through formatted output. Now __make_operation builds a symbolic expression like `((self.expression) <op> (val))` and the new Number defers evaluation until str() / __eq__ asks for it. __str__ evaluates the full symbolic tree once, at the configured precision, so user-facing output is unchanged. __abs__ is left eager: the symbolic form exposes precision artifacts in complex-magnitude identities (e.g. sin²+cos² rounding to 1 + 1 ULP at precision 24) that the eager round-trip used to mask. Arithmetic ops are where chained-precision loss matters; abs is a one-shot call. Regression test in tests/test_number_symbolic.py verifies that the expression is symbolic after arithmetic, that str() still evaluates, and that chained ops produce the correct result. See ai/improvements_2026-05-09.md items #9 and #10. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hozblok
added a commit
that referenced
this pull request
May 12, 2026
…er review) Applies owner review on PR #18: - "Remove Number from this Union. Support only primitives." (line 110) - "Remove this lines." (line 116) — the Number-input branch in _coerce - "Same around Number here" (line 133) — drop the Number-coercion in __make_operation / __prepare_comparison; route through __value._value when __value is a Number, str() it inline when it's a primitive - "Add self._value = Solver(...)()" (line 137) — eagerly evaluate the expression at construction; cache as the canonical value used by arithmetic and comparisons Design pivot. Pre-review, Number was an abstract wrapper that could hold either a literal numeric expression or a symbolic one (with unbound variables, or with arbitrary deferred composition). Post-review, Number represents a *concrete numeric value*. The expression is evaluated by Solver once, at __init__, and the result is cached on self._value. self._expression keeps the original input for round-tripping / debug. Arithmetic and comparison helpers use self._value as the source of truth. Unbound-variable handling: Solver() raises ValueError when called with no bindings on an expression that has variables. Number.__init__ catches that and re-raises as ValueError with a message that names the offending input and the constraint — preserving the original via `raise ... from e` so the Solver-level detail is still reachable through __cause__. Constructor input set narrows to (str, int, float). Number-as-input is gone. bool stays rejected explicitly (int subclass). Tests updated: - tests/test_number_constructor_types.py: drop the inner-Number unwrap tests; add tests for unbound-variable rejection, garbage expression rejection, error-chain preservation, and explicit Number-rejection at both __init__ and _coerce_expression entry points; widen direct-method tests for str inputs. - Existing tests in test_number_reverse_arithmetic.py and the Number cases in test_simple.py still pass — they only used literal numeric or complex expressions, all of which evaluate to concrete values. Coordination: PR #23 (symbolic-accumulation direction) is now in direct conflict with this design and should be closed in favor of this approach. PR #18's body will be updated to flag that alongside the existing PR #17 coordination note. Full pytest suite: 340/340. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
hozblok
added a commit
that referenced
this pull request
May 19, 2026
…er review) Applies owner review on PR #18: - "Remove Number from this Union. Support only primitives." (line 110) - "Remove this lines." (line 116) — the Number-input branch in _coerce - "Same around Number here" (line 133) — drop the Number-coercion in __make_operation / __prepare_comparison; route through __value._value when __value is a Number, str() it inline when it's a primitive - "Add self._value = Solver(...)()" (line 137) — eagerly evaluate the expression at construction; cache as the canonical value used by arithmetic and comparisons Design pivot. Pre-review, Number was an abstract wrapper that could hold either a literal numeric expression or a symbolic one (with unbound variables, or with arbitrary deferred composition). Post-review, Number represents a *concrete numeric value*. The expression is evaluated by Solver once, at __init__, and the result is cached on self._value. self._expression keeps the original input for round-tripping / debug. Arithmetic and comparison helpers use self._value as the source of truth. Unbound-variable handling: Solver() raises ValueError when called with no bindings on an expression that has variables. Number.__init__ catches that and re-raises as ValueError with a message that names the offending input and the constraint — preserving the original via `raise ... from e` so the Solver-level detail is still reachable through __cause__. Constructor input set narrows to (str, int, float). Number-as-input is gone. bool stays rejected explicitly (int subclass). Tests updated: - tests/test_number_constructor_types.py: drop the inner-Number unwrap tests; add tests for unbound-variable rejection, garbage expression rejection, error-chain preservation, and explicit Number-rejection at both __init__ and _coerce_expression entry points; widen direct-method tests for str inputs. - Existing tests in test_number_reverse_arithmetic.py and the Number cases in test_simple.py still pass — they only used literal numeric or complex expressions, all of which evaluate to concrete values. Coordination: PR #23 (symbolic-accumulation direction) is now in direct conflict with this design and should be closed in favor of this approach. PR #18's body will be updated to flag that alongside the existing PR #17 coordination note. Full pytest suite: 340/340. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hozblok
added a commit
that referenced
this pull request
May 19, 2026
…er review) Applies owner review on PR #18: - "Remove Number from this Union. Support only primitives." (line 110) - "Remove this lines." (line 116) — the Number-input branch in _coerce - "Same around Number here" (line 133) — drop the Number-coercion in __make_operation / __prepare_comparison; route through __value._value when __value is a Number, str() it inline when it's a primitive - "Add self._value = Solver(...)()" (line 137) — eagerly evaluate the expression at construction; cache as the canonical value used by arithmetic and comparisons Design pivot. Pre-review, Number was an abstract wrapper that could hold either a literal numeric expression or a symbolic one (with unbound variables, or with arbitrary deferred composition). Post-review, Number represents a *concrete numeric value*. The expression is evaluated by Solver once, at __init__, and the result is cached on self._value. self._expression keeps the original input for round-tripping / debug. Arithmetic and comparison helpers use self._value as the source of truth. Unbound-variable handling: Solver() raises ValueError when called with no bindings on an expression that has variables. Number.__init__ catches that and re-raises as ValueError with a message that names the offending input and the constraint — preserving the original via `raise ... from e` so the Solver-level detail is still reachable through __cause__. Constructor input set narrows to (str, int, float). Number-as-input is gone. bool stays rejected explicitly (int subclass). Tests updated: - tests/test_number_constructor_types.py: drop the inner-Number unwrap tests; add tests for unbound-variable rejection, garbage expression rejection, error-chain preservation, and explicit Number-rejection at both __init__ and _coerce_expression entry points; widen direct-method tests for str inputs. - Existing tests in test_number_reverse_arithmetic.py and the Number cases in test_simple.py still pass — they only used literal numeric or complex expressions, all of which evaluate to concrete values. Coordination: PR #23 (symbolic-accumulation direction) is now in direct conflict with this design and should be closed in favor of this approach. PR #18's body will be updated to flag that alongside the existing PR #17 coordination note. Full pytest suite: 340/340. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Closing as superseded by #37 (the merged version of #18). This branch's premise was to store the unevaluated symbolic expression on The related concerns this branch addressed:
No code remains to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes items #9 and #10 from
ai/improvements_2026-05-09.md(combined per the doc's recommendation: "Combine with #9").Problem. The five arithmetic ops (
+,-,*,/,^) used to evaluate eagerly and stash the formatted result string as the newNumber'sexpression. Each subsequent op re-parsed that rounded string, so chained operations on a high-precisionNumbercould not exceed the precision of formatted output — and string-interpolation of formatted complex outputs back into new expressions was fragile.Fix.
src/formula/formula.py—__make_operationnow builds a symbolic expression((self.expression) <op> (val))and returns a Number that carries the full tree.__str__evaluates that tree once at the configured precision. User-facing output is unchanged.Scope of this PR. Symbolic accumulation applies to arithmetic only.
__abs__is left eager with a code comment explaining why: the symbolic form would expose precision artifacts in complex-magnitude identities (e.g.abs(-sin(pi/8)-i*cos(pi/8))evaluates to1.000…001 + i*(0)at precision 24, not exact 1) that the eager round-trip used to mask via default-format trailing-zero stripping. Arithmetic ops are where chained-precision loss matters; abs is a one-shot call.API note. This changes
Number.expression's meaning after arithmetic — it now carries the symbolic combination, not the eager-evaluated string. Existing callers that read.expressionafter arithmetic will see something like"((1.1) + (1.1))"instead of"2.2". The user-facingstr()output is unchanged.Test. New file
tests/test_number_symbolic.pycovers: expression is symbolic after+, str() still evaluates, deep chains produce correct results, and the eager-abs result still evaluates correctly. Full suite 320/320.