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
is_irreducible() #1129
Comments
This comment has been minimized.
This comment has been minimized.
comment:2
I don't know whether this helps, but here it is: the problem is clearly in factor(), not in is_irreducible(). Now the function factor() first creates the pari polynomial
and then asks pari to factor it. But this is what happens if I try that directly in pari:
So it seems to be an issue with pari, not with sage proper. |
comment:3
Added a fix for this bug. This code called into the pari library function factor0, which was then calling off to factornf. The error coming from factornf is still boggling to me (see note below), but reading the documentation, it mentions that nffactor is in general faster anyway. So I switched the code to use nffactor; this required one small modification elsewhere in the NumberField code. Specifically, F.pari_polynomial would always return a polynomial in "x", but we needed it to be in a different variable (because of Pari's notion of "priority" of variables, basically). So I added an optional argument to that function, switched the factor for polynomials over a NumberField to call into nffactor, and now everything seems to work. Note: the Pari error can be reproduced in gp as follows:
The documentation for factornf says that it uses "Trager's trick" to do factorization over a number field. I think this is just a bug in Pari, which I'm happy to report, as long as someone confirms for me that I'm not doing something stupid (i.e. not knowing how to use Pari correctly). |
comment:4
My results for that gp session don't quite match yours:
This is with 32-bit x86 Debian testing; I get the same results from "sage -gp" and from "/usr/bin/gp" (from the Debian pari-gp package, version 2.3.2-1). |
comment:5
I don't know much about the factornf vs. nffactor, but it seems to work for me. |
comment:6
I'm now getting
|
Attachment: trac_1129.hg.gz |
comment:7
Fixed this patch up a bit. First, at cwitty's suggestion, rewrote it so that it avoids calling nfinit simply for a change in variable names. Also, wrote some (mildly unwieldy) code to deal with cases like factoring x^2-1/3 over a number field generated by x^2-1/4 -- this is particularly troublesome, since Pari only likes to work with integral polynomials. It all seems to work now, though I make no promises about the speed in the non-integral case. If someone notices it being particularly slow in this case, make a trac ticket and we'll start looking into it. |
comment:8
Mostly I like the patch. I do have one question, though: you use the slow path if self.denominator() != 1. Is that actually required? (If so, why?) |
Attachment: 1129_2.patch.gz |
comment:9
Added a patch (that applies after trac_1129.hg) that touches up something suggested by cwitty; namely, if the number field is defined by an integral polynomial, there's no reason to do anything complicated with Pari, even if the polynomial we want to factor is non-integral. |
comment:10
I like the new version. Doctests pass in sage/rings. (Apply trac_1129.hg, then 1129_2.patch) |
comment:11
Merged in 2.8.15.rc0. |
Component: number theory
Issue created by migration from https://trac.sagemath.org/ticket/1129
The text was updated successfully, but these errors were encountered: