fix: MAX_PRECISION should reflect engine capability, not wrapper bindings#43
Draft
hozblok wants to merge 1 commit into
Draft
fix: MAX_PRECISION should reflect engine capability, not wrapper bindings#43hozblok wants to merge 1 commit into
hozblok wants to merge 1 commit into
Conversation
…ings
The pre-fix derivation in src/formula/backend.py was
MAX_PRECISION = max(int(P) for n in dir(_formula)
if n.startswith("mp_real_"))
which made the constant track the highest mp_real_<P> Python wrapper
class on the .pyd. On a .pyd built before the full mp_real ladder was
bound (e.g. one that only exposed mp_real_24), this came out as 24 —
and Solver.__init__'s bounds check rejected anything higher:
>>> from formula import Solver
>>> Solver("sqrt(2)", precision=100)()
ValueError: precision must be in [0, 24] (got 100)
That's wrong: the C++ Formula evaluation engine doesn't depend on the
mp_real_<P> wrappers at all (verified — Formula("sqrt(2)", precision=100)
on the same .pyd runs fine and rounds to 128). The two concepts are
independent: `csconstants::max_precision` is the engine's ceiling;
`py::class_<mp_real<P>>` bindings are a separate Python-side convenience.
Fix:
- main.cpp: expose csconstants::max_precision as `_formula.MAX_PRECISION`.
- backend.py: read `_formula.MAX_PRECISION` with a fallback to 8192 (the
historic engine ceiling) for older .pyds that predate the attr.
Regression test in tests/test_max_precision_is_engine_bound.py confirms
the decoupling: MAX_PRECISION exceeds the highest bound mp_real_<P>
wrapper; Solver accepts precision higher than any wrapper P; the bound
is still enforced at the right ceiling.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Bug
backend.py'sMAX_PRECISIONwas derived from the boundmp_real_<P>wrapper classes:That made the constant track Python wrapper coverage, not the C++ engine's actual precision ceiling. On a
.pydbuilt before the full mp_real ladder was bound (anything pre-#37 — including any locally-installed editable build people might still be running against), onlymp_real_24is exposed, soMAX_PRECISIONcame out as 24.Solver.__init__'s bounds check then rejected anything higher:This is wrong.
Formulaevaluation does not depend onmp_real_<P>wrappers; they're independent. Verified against the same stale.pyd:Fix
Move the constant to where it actually lives — the C++ engine — and re-export.
src/cpp/main.cpp— exposecsconstants::max_precisionas_formula.MAX_PRECISION:src/formula/backend.py— read_formula.MAX_PRECISION, with a fallback to8192(the historic engine ceiling) for older.pydsthat predate the attr:The two concepts are now cleanly separated:
csconstants::max_precision(currentlyp_262144), surfaced as_formula.MAX_PRECISIONpy::class_<mp_real<P>>got registered (currently the fullAllowedPrecisionsSeq)Test
tests/test_max_precision_is_engine_bound.py— 5 cases:MAX_PRECISION >= 8192(the historic floor; never the 24 the bug produced).MAX_PRECISIONexceeds the highest boundmp_real_<P>wrapper P (proves the decoupling on a sparse-wrapper.pyd).Solver("sqrt(2)", precision=100)()succeeds and returns a 100-digit value (the smoking gun — exactly the call that hit the regression).Solver(...)atMAX_PRECISIONruns and returns at leastMAX_PRECISIONdecimal digits.Solver(..., precision=MAX_PRECISION + 1)is still rejected — the bound is still enforced, just at the right ceiling.All 5 pass locally against the stale
.pyd(yieldsMAX_PRECISION = 8192via the fallback).Verification context
Local venv has a
.pydbuilt before #37 (onlymp_real_24exposed;_formula.MAX_PRECISIONnot yet present). My local results:MAX_PRECISION = 24,Solver("sqrt(2)", precision=100)()→ValueError.MAX_PRECISION = 8192(fallback path),Solver("sqrt(2)", precision=100)()→ 100-digit result.Solver("sqrt(2)", precision=8192)()returns 8193 chars (1 + "." + 8191 fractional digits).On a fresh
.pydrebuilt from this branch,_formula.MAX_PRECISIONwill be262144(or whatevermax_precisionis at the time of the build), and the fallback path won't trigger.Out of scope
262144is actually a sensible ceiling — Boost 1.79'scpp_dec_floatstatic-asserts on instantiations that large on at least some toolchains (the master Windows MSVC build currently fails for this reason). That's a separate question; this PR just fixes the wrong-constant-derivation issue.