#23331 closed enhancement (fixed)

Allow exact defining polynomials for p-adic extensions

Reported by: roed Owned by:
Priority: major Milestone: sage-8.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 saraedum)

Currently, the defining polynomial of a p-adic 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()
5-adic 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 saraedum

  • Description modified (diff)

comment:2 Changed 21 months ago by roed

  • Branch set to u/roed/allow_exact_defining_polynomials_for_p_adic_extensions

comment:3 Changed 21 months ago by roed

  • Commit set to dbbc631feff9dbf4f3a37825329263d640290202
  • Status changed from new to needs_review

I ended up doing some additional things:

  • Remove some old code related to lazy p-adics (since I was changing the key format anyway)
  • Change the printing of unramified and eisenstein extensions

New commits:

7e21e77Adding an exact modulus to p-adic rings and fields, changing printing for p-adic extensions, removing some vestigal lazy p-adic code
dbbc631Fix pickles

comment:4 Changed 21 months ago by git

  • Commit changed from dbbc631feff9dbf4f3a37825329263d640290202 to 6cb9d22e018af5672f54d2ba657ae354283098a8

Branch pushed to git repo; I updated commit sha1. New commits:

6cb9d22Remove extraneous newlines in padic_ZZ_pX_FM_element.pyx

comment:5 follow-up: Changed 21 months ago by caruso

Awesome! Some comments through:

  • Why did you implement only exact_field and not exact_ring (for p-adic rings)?
  • Shouldn't we have a coercion map from the exact subfield/subring to the p-adic field/ring?
  • I would say that the methods modulus and defining_polynomial should both by default return a polynomial defined over the p-adic 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 well-defined extension (thanks to Krasner's Lemma)?

comment:6 Changed 21 months ago by roed

  • Dependencies set to #20073

comment:7 in reply to: ↑ 5 ; follow-up: Changed 21 months ago by roed

  • Status changed from needs_review to needs_work

Replying to caruso:

Awesome! Some comments through:

  • Why did you implement only exact_field and not exact_ring (for p-adic rings)?

Two reasons:

  1. We didn't need it for this ticket
  2. Eventually, exact rings would require computing rings of integers in number fields, or at least p-maximal 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 p-adic field/ring?

Yes. I'll add this to the the followup ticket #23471

  • I would say that the methods modulus and defining_polynomial should both by default return a polynomial defined over the p-adic 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 well-defined 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 git

  • Commit changed from 6cb9d22e018af5672f54d2ba657ae354283098a8 to 921af5e95be38203770443d6d9f6e8c9f123d269

Branch pushed to git repo; I updated commit sha1. New commits:

7f96697Fixed typo with variable assignment
bbb7268added doctests
ee461b6merge with develop
8081eabadded doctests after fixing conflicts
063b58cMerge branch 'develop' into t/20073/ticket/20073
48be601Added documentation to verify that the extension has the right defining polynomial
921af5eChanging modulus and defining_polynomial to use an exact keyword

comment:9 Changed 21 months ago by roed

  • Status changed from needs_work to needs_review

comment:10 Changed 21 months ago by saraedum

  • Authors set to David Roe
  • Reviewers set to Julian Rüth

comment:11 Changed 21 months ago by saraedum

  • 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 saraedum

  • Commit changed from 921af5e95be38203770443d6d9f6e8c9f123d269 to 77779eabdebc98cb503fe2e432a90745c8390aab

Positive review once the patchbot is happy.


New commits:

77779eaminor docstring changes

comment:13 Changed 21 months ago by roed

  • 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 saraedum

  • 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 roed

  • 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 git

  • Commit changed from 77779eabdebc98cb503fe2e432a90745c8390aab to 6ba62dd02f9c85ee49ca06a68d99cf724c264b96

Branch pushed to git repo; I updated commit sha1. New commits:

6ba62ddFix SEEALSO again

comment:17 Changed 21 months ago by saraedum

  • Status changed from needs_review to positive_review

comment:18 in reply to: ↑ 7 ; follow-up: Changed 21 months ago by caruso

Replying to roed:

  1. Eventually, exact rings would require computing rings of integers in number fields, or at least p-maximal 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 p-adic 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 roed

Replying to caruso:

Replying to roed:

  1. Eventually, exact rings would require computing rings of integers in number fields, or at least p-maximal 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?

Good point. See #23507.

Shouldn't we have a coercion map from the exact subfield/subring to the p-adic 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.

Last edited 21 months ago by roed (previous) (diff)

comment:20 Changed 21 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, wait for next beta...

comment:21 Changed 21 months ago by git

  • Commit changed from 6ba62dd02f9c85ee49ca06a68d99cf724c264b96 to e9c4c394066d9ce451b500a2ddf9572a488d006f

Branch pushed to git repo; I updated commit sha1. New commits:

e9c4c39Merge 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 git

  • Commit changed from e9c4c394066d9ce451b500a2ddf9572a488d006f to 561f5ac333b00f5e4a2ee70b82e2b5e4965876b3

Branch pushed to git repo; I updated commit sha1. New commits:

561f5acFix doctest errors

comment:23 Changed 21 months ago by roed

  • 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 saraedum

There are patchbot failures. But these seem to be in develop already.

comment:25 Changed 21 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.