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
NumberField(...).pari_polynomial() should return an integral polynomial #11891
Comments
Author: Jeroen Demeyer |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:3
The code looks good, and passes tests, and it means we can at least compute discriminants. The first and second things I try still don't work though, and I'm curious what you think about this:
Side Remarks: 1. I've always intended to just rewrite number fields to completely use
behind the scenes, and do everything that involves PARI in all cases using a monic integral polynomial, and only convert to the user's representation for input and output. I wish I had done that when I was first writing number fields.
I would rather get an error message in the latter case. What do you think? If you agree, could you open a ticket? Anyway, I'll wait for your comments... |
comment:4
Replying to @williamstein:
I never intended this ticket to completely fix non-integral/non-monic number fields. I can have a look at the error messages above and try to fix them, but I also do not want to put too much effort. The main motivation for this ticket was to support #11890, for which it seems to work.
Okay, that should be fixable (currently, we simply substitute instead of creating a polynomial with a given variable). |
comment:5
This seems to cause a doctest failure in elliptic curves. Given the severity of the failure, it is surprising that there is only one failure:
|
comment:6
Attachment: 11891.patch.gz New patch, not yet fully tested. |
This comment has been minimized.
This comment has been minimized.
comment:7
With the new patch, all long tests in |
comment:8
Looks ok to me. fully tested in sage-4.7.1. The code and the tests seem fine. It does not allow to operate in towers of numberfields with nonintegral generators, but neither worked in previous Sage versions. I have a question regarding pari_relative_polynomial in number_field_rel The output of this method will be a monic polynomial. So we may have non-trivial denominators. Is this OK? |
comment:9
Replying to @lftabera:
It's the only way. There is no meaningful definition of the "denominator" of a polynomial over a number field. So in order to get something canonical, let's take the monic polynomial. |
comment:10
Replying to @jdemeyer:
Pedantic point: you can define a denominator ideal sensibly (exercise), just as for number field elements. But that's not really relevant! Any reason why Luis did not give a positive review? I have added his name to the Reviewers field |
Reviewer: Luis Felipe Tabera Alonso |
comment:11
I have not finish yet... |
comment:12
Now I am done, positive review. |
Merged: sage-4.7.3.alpha0 |
Milestone sage-4.7.3 deleted |
Changed merged from sage-4.7.3.alpha0 to sage-4.8.alpha0 |
Mostly, PARI only works with number fields defined by monic integral polynomials. A few functions (like
factornf()
) also work for non-monic polynomials. In these cases, PARI always requires the polynomial to have integer coefficients. So, PARI prefers2*x^2 - 1
overx^2 - 1/2
for defining a number field. TheNumberField
methodpari_polynomial
should reflect this.This change allows one to define number field towers with non-integral polynomials. Example from vanilla sage-4.7.1:
With the patch, this works.
This patch also fixes
by raising
PariError
instead.By chance, it also fixes
_division_field()
for elliptic curves which now always returns an absolute field.CC: @JohnCremona
Component: number fields
Author: Jeroen Demeyer
Reviewer: Luis Felipe Tabera Alonso
Merged: sage-4.8.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/11891
The text was updated successfully, but these errors were encountered: