Skip to content

Don't increment Sturm sign change count twice at each 0#302

Merged
saraedum merged 4 commits into
flatsurf:masterfrom
d-torrance:sturm-count
May 19, 2026
Merged

Don't increment Sturm sign change count twice at each 0#302
saraedum merged 4 commits into
flatsurf:masterfrom
d-torrance:sturm-count

Conversation

@d-torrance
Copy link
Copy Markdown
Contributor

@d-torrance d-torrance commented May 7, 2026

Previously, if 0 appeared in the values of the elements of the Sturm sequence at an endpoint, we counted two sign changes instead of one, e.g., {1, 0, -1, 0, 1, -1, 0} -> 5 changes instead of 3.

Checklist

  • Added an entry in doc/news/.
  • Added a test/benchmark for this change.

Fixes: #301

@d-torrance d-torrance changed the title Sturm count Don't increment Sturm sign change count twice at each 0 May 7, 2026
@d-torrance
Copy link
Copy Markdown
Contributor Author

d-torrance commented May 7, 2026

Hmm this isn't quite right yet...

Fixed -- in the original version of this PR, we weren't counting any roots at 0.

@d-torrance d-torrance marked this pull request as draft May 7, 2026 04:01
d-torrance added 3 commits May 7, 2026 00:19
Previously, if 0 appeared in the values of the elements of Sturm
sequence at an endpoint, we counted two sign changes instead of one,
e.g., {1, 0, -1, 0, 1, -1, 0} -> 5 changes instead of 3.
In addition to the 10 random polynomials, we test one specific one
where the previous algorithm miscounted the number of sign changes.
@d-torrance d-torrance marked this pull request as ready for review May 7, 2026 04:19
@videlec
Copy link
Copy Markdown
Collaborator

videlec commented May 7, 2026

Thanks for looking at it! Code looks code. Let see what CI says.

@saraedum
Copy link
Copy Markdown
Member

The CI needed some updating. The fixes are (hopefully) in #303.

@saraedum saraedum enabled auto-merge May 19, 2026 01:02
@saraedum saraedum merged commit cf39291 into flatsurf:master May 19, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

t-num_real_roots_0_1 fails with FLINT 3.5.0

3 participants