Skip to content

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Sep 20, 2025

As in the title. Fix #37068. Can be removed once the upstream issue linbox-team/linbox#53 is fixed.

It's possible to do a more expensive check (e.g. check if the matrix satisfy the minpoly with something like Freivalds' algorithm), but I guess this suffice (and it's very cheap).

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Sep 20, 2025

Documentation preview for this PR (built with commit e5ab51a; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor

If that (leading? last?) coefficient guaranteed to be != 1 if the algorithm fails?

I think the "linbox is unreliable" comments would be more helpful around while True, since that's the unexpected part.

@user202729
Copy link
Contributor Author

If that (leading? last?) coefficient guaranteed to be != 1 if the algorithm fails?

my understanding is that the algorithm fails if it incorrectly choose a prime that makes the minpoly mod that prime's degree less than the minpoly over ℤ's degree. If that happens (and at least a good prime is selected) then the leading coefficient must be ≡ 1 mod the good primes and ≡ 0 mod the bad prime, so it cannot be 1.

A corner case I can think of is what if all primes selected are bad, but I think this is impossible because of the Hadamard bound.

@orlitzky
Copy link
Contributor

So the size of the coefficient list is known beforehand? I was thinking it might still output a monic polynomial but of a lesser degree, which, if the zeros are omitted, could still have lc() == 1.

Regardless, this can't be any worse, and fixes the reproducible case that we have. Let's try it.

@user202729 user202729 added the p: CI Fix merged before running CI tests label Sep 21, 2025
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 22, 2025
sagemathgh-40856: Workaround for linbox charpoly/minpoly issues
    
As in the title. Fix sagemath#37068. Can
be removed once the upstream issue https://github.com/linbox-
team/linbox/issues/53 is fixed.

It's possible to do a more expensive check (e.g. check if the matrix
satisfy the minpoly with something like Freivalds' algorithm), but I
guess this suffice (and it's very cheap).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40856
Reported by: user202729
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 24, 2025
sagemathgh-40856: Workaround for linbox charpoly/minpoly issues
    
As in the title. Fix sagemath#37068. Can
be removed once the upstream issue https://github.com/linbox-
team/linbox/issues/53 is fixed.

It's possible to do a more expensive check (e.g. check if the matrix
satisfy the minpoly with something like Freivalds' algorithm), but I
guess this suffice (and it's very cheap).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40856
Reported by: user202729
Reviewer(s):
@vbraun vbraun merged commit 487663c into sagemath:develop Sep 27, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: CI Fix merged before running CI tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random test failure in charpoly_frobenius
3 participants