Repository navigation
fix(polynomial): normalize Sturm chain entries - #360
Open
KSHITIZ6341 wants to merge 2 commits into
Open
KSHITIZ6341 wants to merge 2 commits into
KSHITIZ6341 wants to merge 2 commits into
Conversation
5 tasks done
This branch has not been deployed
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.
What & why
Fixes #295. A degree-12 polynomial built from roots ±1 through ±6 has entirely finite coefficients after scaling by
3e32inf32or1.5e302inf64, but root counting currently returnsOk(0). Normalize the initial polynomial before differentiating, its derivative, and each subsequent negated remainder by their positive maximum coefficient magnitude. This preserves Sturm signs and prevents coefficient scaling from silently discarding roots. Divide coefficients directly to avoid overflowing the reciprocal of a tiny norm; reject non-finite intermediate entries.The new f32/f64 round-trip tests cover ordinary, tiny, huge, and negative multipliers, root counts, and root isolation. Both regressions were verified to fail on the unchanged implementation. Codex assisted with investigation, implementation, and validation.
Checklist
cargo test(38 unit tests, 1477 suite tests, 463 doctests) andcargo clippy --all-targetspass locallyunwrap/expect/panicon library pathscargo fmt --checkandgit diff --checkEmbedded text-size baseline
The
thumbv7em-none-eabihfsmoke image needs an updated.textbaseline for the checked Sturm-chain normalization. Perci/README.md, only this target's measuredtext_bytesrow changes, to 201812; the 10% tolerance and all stack budgets remain unchanged.The pre-change merged PR358 job already failed this row: 201028 bytes on Rust 1.99.0 versus the existing 200046-byte limit. The initial PR360 job measured 201812 bytes on Rust 1.99.1; both measured 3119 bytes of peak stack. These CI builds use different compiler patch versions, so their 784-byte difference is not a controlled comparison.
A controlled local release build with Rust 1.96.1 and
thumbv7em-none-eabihfmeasured 191492 bytes at base a33031d and 192692 bytes with this fix (+1200 bytes). The additional finite checks and positive rescaling prevent finite polynomial inputs from silently losing all real roots. The measured CI value is proposed as a reviewed baseline; no numerical checks or budget tolerance are relaxed.The updated hardware-FPU budget check now passes. The remaining RISC-V runtime assembly failure (
.cfi_startproc/unfinished frame) is also present in merged PR358's baseline job; its runtime/toolchain is unchanged by this PR.