Normalize root-counting chain entries to survive f32 at high degree - #344
Open
naseem173 wants to merge 5 commits into
Open
Normalize root-counting chain entries to survive f32 at high degree#344naseem173 wants to merge 5 commits into
naseem173 wants to merge 5 commits into
Conversation
Normalize coefficients to prevent overflow and underflow.
Added tests for counting real roots of high-degree polynomials, including a degree-12 polynomial with close pairs of roots to ensure accuracy.
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.
root_counting_chainbuilt up each new Sturm-sequence entry from raw pseudo-division remainders with no rescaling, so the coefficient magnitudes could drift geometrically down the chain. For a degree-12 polynomial with two close pairs of roots (0.0001 apart), that drift ran anf32chain entry to exact zero partway through, andcount_real_rootsreturned 8 instead of 12 -- the same failure mode described in the issue.The fix renormalizes each new chain entry to unit max-norm (divide every coefficient by the largest absolute coefficient) right after it is built.
sign_changesis the only thing that reads the chain back out, and dividing by a positive scalar never changes a sign, so this is a no-op for correctness on every case that already worked -- it only prevents the coefficients from running out of range.Added
count_real_roots_survives_high_degree_f32/_f64, which build the degree-12 polynomial described above and assert the count is 12 at bothf32andf64. Thef32variant fails onmain(finds 8) and passes with this change; thef64variant passes either way, sincef64has enough range that this particular case doesn't run out of itChecklist
cargo test+cargo clippy --all-targetsclean locallyunwrap/expect/panicon library paths (typed errors instead)