Opened 5 years ago
Closed 5 years ago
#23331 closed enhancement (fixed)
Allow exact defining polynomials for padic extensions
Reported by:  David Roe  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  padics  Keywords:  sd87 
Cc:  Julian Rüth  Merged in:  
Authors:  David Roe  Reviewers:  Julian Rüth 
Report Upstream:  N/A  Work issues:  
Branch:  561f5ac (Commits, GitHub, GitLab)  Commit:  561f5ac333b00f5e4a2ee70b82e2b5e4965876b3 
Dependencies:  #20073  Stopgaps: 
Description (last modified by )
Currently, the defining polynomial of a padic extension always has coefficients over the base ring:
sage: R.<x> = ZZ[] sage: W.<w> = Zp(5).extension(x^2  5) sage: W.defining_polynomial().base_ring() 5adic Ring with capped relative precision 20
This is fine, one would expect the defining polynomial to have coefficients in the base ring but we would also like to have access to the exact polynomial. To make this consistent, we should always keep two polynomials: a defining polynomial and an exact polynomial.
The defining polynomial has coefficients over the base ring. It is not part of the key used in the factory as it can be recovered from the exact polynomial.
The exact polynomial has coefficients in a number field: For extension of Zp/Qp, the coefficients are in QQ. For two step extensions, the coefficients are in the absolute number field defined by the exact polynomial of the base ring. For more general extensions, the coefficients are in the tower of number fields, defined by the exact polynomials.
The exact polynomial is part of the key used in the factory. Two fields with different exact polynomial but same defining polynomial are different. This makes sense because they behave differently with respect to change()
when changing the precision.
For extensions constructed from a modulus with inexact coefficients (in the base ring) we just set the exact polynomial to an approximation of the modulus.
Change History (25)
comment:1 Changed 5 years ago by
Description:  modified (diff) 

comment:2 Changed 5 years ago by
Branch:  → u/roed/allow_exact_defining_polynomials_for_p_adic_extensions 

comment:3 Changed 5 years ago by
Commit:  → dbbc631feff9dbf4f3a37825329263d640290202 

Status:  new → needs_review 
comment:4 Changed 5 years ago by
Commit:  dbbc631feff9dbf4f3a37825329263d640290202 → 6cb9d22e018af5672f54d2ba657ae354283098a8 

Branch pushed to git repo; I updated commit sha1. New commits:
6cb9d22  Remove extraneous newlines in padic_ZZ_pX_FM_element.pyx

comment:5 followup: 7 Changed 5 years ago by
Awesome! Some comments through:
 Why did you implement only
exact_field
and notexact_ring
(for padic rings)?  Shouldn't we have a coercion map from the exact subfield/subring to the padic field/ring?
 I would say that the methods
modulus
anddefining_polynomial
should both by default return a polynomial defined over the padic field and accept an attribute specifying whether the user wants an exact defining polynomial (I believe that having different behaviour for these two methods is quite confusing).  Shouldn't we check that the precision of the defining polynomial is enough to welldefined extension (thanks to Krasner's Lemma)?
comment:6 Changed 5 years ago by
Dependencies:  → #20073 

comment:7 followup: 18 Changed 5 years ago by
Status:  needs_review → needs_work 

Replying to caruso:
Awesome! Some comments through:
 Why did you implement only
exact_field
and notexact_ring
(for padic rings)?
Two reasons:
 We didn't need it for this ticket
 Eventually, exact rings would require computing rings of integers in number fields, or at least pmaximal orders. We don't want to solve a hard computational problem problem to do something trivial.
 Shouldn't we have a coercion map from the exact subfield/subring to the padic field/ring?
Yes. I'll add this to the the followup ticket #23471
 I would say that the methods
modulus
anddefining_polynomial
should both by default return a polynomial defined over the padic field and accept an attribute specifying whether the user wants an exact defining polynomial (I believe that having different behaviour for these two methods is quite confusing).
I had a conversation with Julian, Marc and Rob and we agree.
 Shouldn't we check that the precision of the defining polynomial is enough to welldefined extension (thanks to Krasner's Lemma)?
Yes, but I think that this should be saved for a followup ticket #23480 (Julian has code for doing this, but it depends on the MacLane? package).
comment:8 Changed 5 years ago by
Commit:  6cb9d22e018af5672f54d2ba657ae354283098a8 → 921af5e95be38203770443d6d9f6e8c9f123d269 

Branch pushed to git repo; I updated commit sha1. New commits:
7f96697  Fixed typo with variable assignment

bbb7268  added doctests

ee461b6  merge with develop

8081eab  added doctests after fixing conflicts

063b58c  Merge branch 'develop' into t/20073/ticket/20073

48be601  Added documentation to verify that the extension has the right defining polynomial

921af5e  Changing modulus and defining_polynomial to use an exact keyword

comment:9 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:10 Changed 5 years ago by
Authors:  → David Roe 

Reviewers:  → Julian Rüth 
comment:11 Changed 5 years ago by
Branch:  u/roed/allow_exact_defining_polynomials_for_p_adic_extensions → u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions 

comment:12 Changed 5 years ago by
Commit:  921af5e95be38203770443d6d9f6e8c9f123d269 → 77779eabdebc98cb503fe2e432a90745c8390aab 

comment:13 Changed 5 years ago by
Branch:  u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions → u/roed/allow_exact_defining_polynomials_for_p_adic_extensions 

comment:14 Changed 5 years ago by
Branch:  u/roed/allow_exact_defining_polynomials_for_p_adic_extensions → u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions 

comment:15 Changed 5 years ago by
Branch:  u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions → u/roed/allow_exact_defining_polynomials_for_p_adic_extensions 

comment:16 Changed 5 years ago by
Commit:  77779eabdebc98cb503fe2e432a90745c8390aab → 6ba62dd02f9c85ee49ca06a68d99cf724c264b96 

Branch pushed to git repo; I updated commit sha1. New commits:
6ba62dd  Fix SEEALSO again

comment:17 Changed 5 years ago by
Status:  needs_review → positive_review 

comment:18 followup: 19 Changed 5 years ago by
Replying to roed:
 Eventually, exact rings would require computing rings of integers in number fields, or at least pmaximal orders. We don't want to solve a hard computational problem problem to do something trivial.
I don't unserstand why implementing exact_ring
would require something more than just calling ZZ.extension(...)
that creates the order in a number field defined by the given polynomial. We do not care if it is not the maximal order, don't we?
Shouldn't we have a coercion map from the exact subfield/subring to the padic field/ring?
Yes. I'll add this to the the followup ticket #23471
Is it in it? (I ask the question because this ticket is not marked as a dependency of #23471.)
comment:19 Changed 5 years ago by
Replying to caruso:
Replying to roed:
 Eventually, exact rings would require computing rings of integers in number fields, or at least pmaximal orders. We don't want to solve a hard computational problem problem to do something trivial.
I don't unserstand why implementing
exact_ring
would require something more than just callingZZ.extension(...)
that creates the order in a number field defined by the given polynomial. We do not care if it is not the maximal order, don't we?
Good point. See #23507.
Shouldn't we have a coercion map from the exact subfield/subring to the padic field/ring?
Yes. I'll add this to the the followup ticket #23471
Is it in it? (I ask the question because this ticket is not marked as a dependency of #23471.)
This ticket is an indirect dependency, through #20310.
comment:20 Changed 5 years ago by
Status:  positive_review → needs_work 

Merge conflict, wait for next beta...
comment:21 Changed 5 years ago by
Commit:  6ba62dd02f9c85ee49ca06a68d99cf724c264b96 → e9c4c394066d9ce451b500a2ddf9572a488d006f 

Branch pushed to git repo; I updated commit sha1. New commits:
e9c4c39  Merge branch 'u/roed/allow_exact_defining_polynomials_for_p_adic_extensions' of git://trac.sagemath.org/sage into t/23331/allow_exact_defining_polynomials_for_p_adic_extensions

comment:22 Changed 5 years ago by
Commit:  e9c4c394066d9ce451b500a2ddf9572a488d006f → 561f5ac333b00f5e4a2ee70b82e2b5e4965876b3 

Branch pushed to git repo; I updated commit sha1. New commits:
561f5ac  Fix doctest errors

comment:23 Changed 5 years ago by
Status:  needs_work → positive_review 

I fixed the merge conflict and ran all tests (fixing two).
comment:24 Changed 5 years ago by
There are patchbot failures. But these seem to be in develop already.
comment:25 Changed 5 years ago by
Branch:  u/roed/allow_exact_defining_polynomials_for_p_adic_extensions → 561f5ac333b00f5e4a2ee70b82e2b5e4965876b3 

Resolution:  → fixed 
Status:  positive_review → closed 
I ended up doing some additional things:
New commits:
Adding an exact modulus to padic rings and fields, changing printing for padic extensions, removing some vestigal lazy padic code
Fix pickles