Opened 22 months ago
Closed 21 months ago
#23331 closed enhancement (fixed)
Allow exact defining polynomials for padic extensions
Reported by:  roed  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  padics  Keywords:  sd87 
Cc:  saraedum  Merged in:  
Authors:  David Roe  Reviewers:  Julian Rüth 
Report Upstream:  N/A  Work issues:  
Branch:  561f5ac (Commits)  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 21 months ago by
 Description modified (diff)
comment:2 Changed 21 months ago by
 Branch set to u/roed/allow_exact_defining_polynomials_for_p_adic_extensions
comment:3 Changed 21 months ago by
 Commit set to dbbc631feff9dbf4f3a37825329263d640290202
 Status changed from new to needs_review
comment:4 Changed 21 months ago by
 Commit changed from dbbc631feff9dbf4f3a37825329263d640290202 to 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 21 months 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 21 months ago by
 Dependencies set to #20073
comment:7 in reply to: ↑ 5 ; followup: ↓ 18 Changed 21 months ago by
 Status changed from needs_review to 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 21 months ago by
 Commit changed from 6cb9d22e018af5672f54d2ba657ae354283098a8 to 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 21 months ago by
 Status changed from needs_work to needs_review
comment:10 Changed 21 months ago by
 Reviewers set to Julian Rüth
comment:11 Changed 21 months ago by
 Branch changed from u/roed/allow_exact_defining_polynomials_for_p_adic_extensions to u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions
comment:12 Changed 21 months ago by
 Commit changed from 921af5e95be38203770443d6d9f6e8c9f123d269 to 77779eabdebc98cb503fe2e432a90745c8390aab
comment:13 Changed 21 months ago by
 Branch changed from u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions to u/roed/allow_exact_defining_polynomials_for_p_adic_extensions
comment:14 Changed 21 months ago by
 Branch changed from u/roed/allow_exact_defining_polynomials_for_p_adic_extensions to u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions
comment:15 Changed 21 months ago by
 Branch changed from u/saraedum/allow_exact_defining_polynomials_for_p_adic_extensions to u/roed/allow_exact_defining_polynomials_for_p_adic_extensions
comment:16 Changed 21 months ago by
 Commit changed from 77779eabdebc98cb503fe2e432a90745c8390aab to 6ba62dd02f9c85ee49ca06a68d99cf724c264b96
Branch pushed to git repo; I updated commit sha1. New commits:
6ba62dd  Fix SEEALSO again

comment:17 Changed 21 months ago by
 Status changed from needs_review to positive_review
comment:18 in reply to: ↑ 7 ; followup: ↓ 19 Changed 21 months 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 in reply to: ↑ 18 Changed 21 months 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 21 months ago by
 Status changed from positive_review to needs_work
Merge conflict, wait for next beta...
comment:21 Changed 21 months ago by
 Commit changed from 6ba62dd02f9c85ee49ca06a68d99cf724c264b96 to 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 21 months ago by
 Commit changed from e9c4c394066d9ce451b500a2ddf9572a488d006f to 561f5ac333b00f5e4a2ee70b82e2b5e4965876b3
Branch pushed to git repo; I updated commit sha1. New commits:
561f5ac  Fix doctest errors

comment:23 Changed 21 months ago by
 Status changed from needs_work to positive_review
I fixed the merge conflict and ran all tests (fixing two).
comment:24 Changed 21 months ago by
There are patchbot failures. But these seem to be in develop already.
comment:25 Changed 21 months ago by
 Branch changed from u/roed/allow_exact_defining_polynomials_for_p_adic_extensions to 561f5ac333b00f5e4a2ee70b82e2b5e4965876b3
 Resolution set to fixed
 Status changed from positive_review to 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