Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in charpoly over discrete valuation rings #31753

Closed
xcaruso opened this issue Apr 29, 2021 · 20 comments
Closed

Bug in charpoly over discrete valuation rings #31753

xcaruso opened this issue Apr 29, 2021 · 20 comments

Comments

@xcaruso
Copy link
Contributor

xcaruso commented Apr 29, 2021

There is a bug in the computation of the characteristic polynomial over a discrete valuation ring.

sage: R.<t> = GF(5)[[]]
sage: M = matrix(3, 3, [1, t + O(t^3), t^2,
....:                   1 + t + O(t^3), 2 + t^2, 3 + 2*t + O(t^3),
....:                   t - t^2, 2*t, 1 + t])
sage: M.charpoly()
Traceback (most recent call last):
...
TypeError: 'PlusInfinity' object cannot be interpreted as an integer
Hessenberg form only possible for matrices over a field

Expected:

sage: M.charpoly()
x^3 + (1 + 4*t + 4*t^2 + O(t^3))*x^2 + (t + 2*t^2 + O(t^3))*x + 3 + 2*t^2 + O(t^3)

This ticket fixes that.

CC: @sagetrac-rpages @slel

Component: linear algebra

Keywords: characteristic polynomial, charpoly

Author: Xavier Caruso

Branch/Commit: 41b3880

Reviewer: Samuel Lelièvre

Issue created by migration from https://trac.sagemath.org/ticket/31753

@xcaruso xcaruso added this to the sage-9.4 milestone Apr 29, 2021
@xcaruso
Copy link
Contributor Author

xcaruso commented Apr 29, 2021

Branch: u/caruso/charpoly

@xcaruso
Copy link
Contributor Author

xcaruso commented Apr 29, 2021

Commit: de32db6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Changed commit from de32db6 to ca3285a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

ca3285ahandle case where precision_relative is infinite

@xcaruso
Copy link
Contributor Author

xcaruso commented Apr 29, 2021

Author: Xavier Caruso

@slel
Copy link
Member

slel commented Apr 29, 2021

Reviewer: Samuel Lelièvre, ...

@slel
Copy link
Member

slel commented Apr 29, 2021

comment:5

a.

The patchbot reports an incorrect Trac link. To fix it:

-    We check that trac:`31753` is resolved::
+    We check that :trac:`31753` is resolved::

b.

Your fix for the bug seems to consist in removing m from a cdef declaration.

Can you explain why that caused a problem, and why this is a good solution?

But probably someone who knows more Cython than me should review that part.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

e43030cmissing colon

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Changed commit from ca3285a to e43030c

@xcaruso
Copy link
Contributor Author

xcaruso commented Apr 29, 2021

comment:7

Replying to @slel:

Your fix for the bug seems to consist in removing m from a cdef declaration.
Can you explain why that caused a problem, and why this is a good solution?

It's just because precision_relative can return +Infinity which cannot be cast to an integer (in fact an element of type Py_ssize_t). Removing the declaration, we let python handle this.

@slel
Copy link
Member

slel commented Apr 29, 2021

comment:9

Right, m can be infinity. That's why maxi is also
left out of the cdef. Is there a cdef type for
Python or Sage objects that could be used?
Perhaps not crucial, as the inner loop matters most.

Still, about cdef lines, maybe remove the one about c
and add entry to the RingElement one?

-    cdef Matrix_generic_dense c
-    cdef RingElement pivot, inv, scalar
+    cdef RingElement entry, pivot, inv, scalar

Commit 28a35a97
from #30892 added the cdef ... c line while removing the use of c.

@xcaruso
Copy link
Contributor Author

xcaruso commented Apr 29, 2021

comment:10

Replying to @slel:

-    cdef Matrix_generic_dense c
-    cdef RingElement pivot, inv, scalar
+    cdef RingElement entry, pivot, inv, scalar

Good point!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Changed commit from e43030c to 41b3880

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

41b3880better cdef

@slel
Copy link
Member

slel commented Apr 29, 2021

comment:12

Since this fixes a bug, maybe it can still go in Sage 9.3.

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Apr 29, 2021

Changed reviewer from Samuel Lelièvre, ... to Samuel Lelièvre

@slel
Copy link
Member

slel commented Apr 29, 2021

Changed keywords from characteristic polynomial to characteristic polynomial, charpoly

@fchapoton
Copy link
Contributor

comment:14

milestone to 9.4, as 9.3 has been released

@fchapoton fchapoton modified the milestones: sage-9.3, sage-9.4 May 10, 2021
@vbraun
Copy link
Member

vbraun commented Jun 19, 2021

Changed branch from u/caruso/charpoly to 41b3880

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants