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

linbox minpoly over small finite fields is TOTALLY BROKEN #6296

Closed
williamstein opened this issue Jun 15, 2009 · 11 comments
Closed

linbox minpoly over small finite fields is TOTALLY BROKEN #6296

williamstein opened this issue Jun 15, 2009 · 11 comments

Comments

@williamstein
Copy link
Contributor



On Wed, Jun 10, 2009 at 6:03 PM, Yann<yannlaiglechapuy@gmail.com> wrote:
>
> ----------------------------------------------------------------------
> | Sage Version 4.0.1, Release Date: 2009-06-06                       |
> | Type notebook() for the GUI, and license() for information.        |
> ----------------------------------------------------------------------
sage: A=matrix(GF(3),2,[0,0,1,2])
sage: R.<x>=GF(3)[]
sage: D={ x:0 , x+1:0 , x^2+x:0 }
sage: for i in range(10000): D[A._minpoly_linbox()]+=1

sage: D
{x: 38266, x + 1: 29397, x^2 + x: 32337}
>


You're absolutely right!  This *sucks* -- it seems like nothing we have ever wrapped in Linbox is right at first.  Hopefully the issue is that somehow the algorithm is only supposed to be probabilistic, and we're just misusing it in sage (quite possible). 

Component: linear algebra

Author: William Stein

Reviewer: Yann Laigle-Chapuy

Merged: sage-4.3.3.alpha0

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

@williamstein
Copy link
Contributor Author

comment:1

from a linbox devel:

Well, I think this was corrected in linbox-1.1.6:

The minpoly algorithm used depends on which method you are using from
LinBox of course but,
If you use the solution "minpoly" you will get the blackbox algorithm
(just like if you specify "minpoly(pol, mat, Method::Blackbox())")
then (since sept 2008 and 1.1.6) we will end up using an extension field
to compute the minpoly (on my machine it will be GF(3^10)) and then
I e.g. got the following result for one try (the algorithm is still
probabilistic, but has a much larger success rate, roughly around 1/3^10):

 > 99993 minimal Polynomials are x^2 +x, 3 minimal polynomial are x+1, 4
minimal polynomials are x

Now for a so small matrix it could be better to use a dense version,
which can be called by "minpoly(pol,mat,Method::Elimination())".
If i am correct this dense version is also probabilistic (choice of the
Krylov non-zero vector) and therefore should also pick vectors from an
extension.
This is not the case in 1.1.6.
Clément can you confirm this ? If so it should be easy to fix, the same
way we fixed Wiedemann.

For your example matrix in some of the cases, when vectors [1,1], and
[2,2] are chosen the Krylov space has rank 1, whereas for other non zero
vectors  it has rank 2 and
thus the dense minbpoly will be x^2+x or x+1 ...

btw, the returned polynomial is always a factor of the true polynomial,
therefore to get a 1/3^{10k} probability  of success it will be
sufficient to perform the lcm of k runs.

Best,

--
                                       Jean-Guillaume Dumas.

My remarks

Hi Yann (and sage-support),

This is from a linbox developer (see below).   This will be fixed by:

 (1) upgrading -- actually, we *already* use linbox-1.1.6 in sage, so ...

 (2) making it so minpoly by default just raises a NotImplementedError, however
   minpoly(proof=False) will call minpoly a bunch of times and return
the lcm of the
   results.

It turns out that maybe linbox doesn't seem to have a proof=True
minpoly algorithm yet (they are hard to write), so our wrapping of
linbox is wrong, given that in Sage the default is proof=True
everywhere.

Yann -- if you want to work on improving the situation wrt any of the
above, please do.

William

@williamstein

This comment has been minimized.

@williamstein
Copy link
Contributor Author

comment:3

Attachment: trac_6296.patch.gz

@williamstein

This comment has been minimized.

@sagetrac-ylchapuy
Copy link
Mannequin

sagetrac-ylchapuy mannequin commented Feb 2, 2010

comment:4

We should at least take the lcm of the results so far:

line 974: g = g.lcm(self._minpoly_linbox(var)

otherwise, it seems ok.

Yann

@williamstein
Copy link
Contributor Author

Attachment: trac_6296-part2.patch.gz

this is part 2

@sagetrac-ylchapuy
Copy link
Mannequin

sagetrac-ylchapuy mannequin commented Feb 7, 2010

comment:5

Ok positive review.

As an aside, is their any reason the result is cached but never fetched?

Yann

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 11, 2010

Merged: sage-4.3.3.alpha0

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 11, 2010

Author: William Stein

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 11, 2010

Reviewer: Yann Laigle-Chapuy

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 11, 2010

comment:6

Please remember to update the relevant ticket fields --- the release
managers use an automated script to generate lists of merged tickets.

@qed777 qed777 mannequin removed the s: positive review label Feb 11, 2010
@qed777 qed777 mannequin closed this as completed Feb 11, 2010
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

1 participant