Opened 11 years ago

Closed 4 years ago

#8240 closed defect (fixed)

cannot coerce p-adic field into residue field

Reported by: dmharvey Owned by: roed
Priority: major Milestone: sage-8.1
Component: padics Keywords:
Cc: roed, robertwb, saraedum, caruso, kedlaya Merged in:
Authors: David Roe Reviewers: Kiran Kedlaya
Report Upstream: N/A Work issues:
Branch: 1fc48d1 (Commits, GitHub, GitLab) Commit: 1fc48d1a6f4baf9625c291fdb8eae0f8d1109d3c
Dependencies: #14825 Stopgaps:

Status badges

Description

sage: K.<a> = Qq(25)
sage: F = K.residue_field()
sage: F(a)
Traceback (click to the left of this block for traceback)
...
TypeError: unable to coerce

Perhaps this is a "feature request", but it seems like a pretty basic feature...

(It works fine for prime fields)

Attachments (4)

8240.patch (66.1 KB) - added by roed 9 years ago.
padic base coercion
8240_ext1.patch (46.5 KB) - added by roed 9 years ago.
coercion for extensions, part 1
8240_ext2.patch (23.5 KB) - added by roed 9 years ago.
coercion for extensions, part 2
8240_vs_14825.diff (56.7 KB) - added by roed 4 years ago.
Diff against #14825 for ease of review

Download all attachments as: .zip

Change History (47)

comment:1 Changed 11 years ago by robertwb

  • Cc robertwb added

Changed 9 years ago by roed

padic base coercion

Changed 9 years ago by roed

coercion for extensions, part 1

Changed 9 years ago by roed

coercion for extensions, part 2

comment:2 Changed 9 years ago by roed

  • Dependencies set to #12062, #12053, #12064, #12077

I've updated the dependencies. If you care about this ticket, review them!

Note that the 8240.patch is a duplicate of #12077 and should be removed. 8240_ext1.patch and 8240_ext2.patch are in progress: I basically didn't finish writing them. Feel free to add to these patches in the same vein as #12077.

comment:3 Changed 4 years ago by roed

  • Branch set to u/roed/residue_coercion

comment:4 Changed 4 years ago by git

  • Commit set to c456a748154ba2352e8475b89a5bdd2a56973dc6

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

c456a74Adding lots of documentation, fixing doctests and problems

comment:5 Changed 4 years ago by roed

  • Dependencies #12062, #12053, #12064, #12077 deleted
  • Work issues set to resolve doctest errors from incorrect reductions elsewhere in Sage

Last 10 new commits:

bd15d71add exact keyword
99dccf6Merge branch 't/23331/allow_exact_defining_polynomials_for_p_adic_extensions' into t/20310/change_precision
ef4ed33Fix SEEALSO:
1754b44Fix exact=True errors in the right branch
a29bf7aMerge branch 't/20310/change_precision' into t/23471/padic_ext_conversion
f985aa0Add ability to call with arguments
9763d7aFix implementation of _call_with_args
3b52f2cFix bug from args being a tuple, not a list
1944974Add morphisms to and from residue field (still need doctests)
c456a74Adding lots of documentation, fixing doctests and problems

comment:6 Changed 4 years ago by roed

sage -t --warn-long 53.3 src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py  # 27 doctests failed
sage -t --warn-long 53.3 src/sage/modular/btquotients/pautomorphicform.py  # 24 doctests failed
sage -t --warn-long 53.3 src/sage/schemes/elliptic_curves/padics.py  # 1 doctest failed
sage -t --warn-long 53.3 src/sage/modular/btquotients/btquotient.py  # 4 doctests failed
sage -t --warn-long 53.3 src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py  # 4 doctests failed

comment:7 Changed 4 years ago by git

  • Commit changed from c456a748154ba2352e8475b89a5bdd2a56973dc6 to 550a831d92fd7055389ab98bcf9ea3c3e57f272f

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

550a831Merge branch 'u/roed/residue_coercion' of git://trac.sagemath.org/sage into t/8240/residue_coercion

comment:8 Changed 4 years ago by roed

  • Authors set to David Roe
  • Cc saraedum caruso kedlaya added

So, there are a number of failures coming from following change

sage: R = Zp(5); S = Zmod(5^5)
sage: a = R(1, 3); a
1 + O(5^3)
sage: S(a)
Traceback (most recent call last):
...
PrecisionError: Not enough precision to determine reduction.

Previously, Sage performed this reduction without complaint. Some questions:

  • Do we want a coercion from R to S? Mathematically, this coercion is very similar to the one from ZZ to Zmod(N), so I think the answer is yes.
  • If there is a coercion, what should its behavior be on elements without enough precision, as in the example above? A PrecisionError seems reasonable, but normally coercions don't raise errors....
  • The coercion model demands that if there is a coercion between two parents, then that morphism should be used as the conversion as well (this is implemented in discover_convert_map, which calls _internal_coerce_map_from as the first step). So we can't have different behavior for the coercion and the conversion from R to S.
  • It's pretty annoying to have these kind of PrecisionErrors in conversions. For example, conversions are used in change_ring on polynomials, and there are plenty of examples in the Sage library where change_ring that trigger this kind of PrecisionError.

Any suggestions?

comment:9 Changed 4 years ago by robharron

I just checked the following:

sage: R1 = RealIntervalField(10)
sage: C1 = ComplexIntervalField(10)
sage: C1.coerce_map_from(R1)

Call morphism:
  From: Real Interval Field with 10 bits of precision
  To:   Complex Interval Field with 10 bits of precision
sage: C1 = ComplexIntervalField(11)
sage: C1.coerce_map_from(R1)
sage: C1.convert_map_from(R1)

Call morphism:
  From: Real Interval Field with 10 bits of precision
  To:   Complex Interval Field with 11 bits of precision

So, there's a precedent in the archimedean code to only have a convert map, not a coerce map, when mapping from a lower precision to a higher precision. For Zp(5) to Zmod(55), you're mapping an inexact ring to an exact ring, so this would suggest that there should be no coercion, only a conversion.

comment:10 follow-up: Changed 4 years ago by roed

Yeah, just having a conversion seems like the simplest solution. I think it may be reasonable to have a coercion to the residue field though?

comment:11 in reply to: ↑ 10 Changed 4 years ago by robharron

Replying to roed:

Yeah, just having a conversion seems like the simplest solution. I think it may be reasonable to have a coercion to the residue field though?

Yeah, I think coercion to the residue field makes sense, though a PrecisionError? will still need to be raised for O(50); I think that's reasonable since O(50) basically means you have no idea what element of Zp you have!

comment:12 Changed 4 years ago by git

  • Commit changed from 550a831d92fd7055389ab98bcf9ea3c3e57f272f to c08c76432d957b62d799581051cbab40c9766de2

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

fa53969Fix some doctest errors
fd3ff02Merge branch 'develop' into t/8240/residue_coercion
c08c764Add check_prec keyword for residue(), fix error in padic_ZZ_pX_* constructors

comment:13 Changed 4 years ago by roed

  • Status changed from new to needs_review
  • Work issues resolve doctest errors from incorrect reductions elsewhere in Sage deleted

comment:14 follow-ups: Changed 4 years ago by caruso

IMO, It's a kinda weird to promote reduction modulo p to a coercion map but not reduction modulo pn.

Last edited 4 years ago by caruso (previous) (diff)

comment:15 in reply to: ↑ 14 Changed 4 years ago by roed

Replying to caruso:

IMO, It's a kinda weird to promote reduction modulo p to a coercion map but not reduction modulo pn.

I agree that it's kind of weird. I'd really like for them both to be coercions, but I explain above why that doesn't seem to be possible.

I think it's probably okay to also delete the coercion to the residue field. For integers, you really want to be able to ask for a+1 if a is an element of Zmod(N), but I think that this is a lot less important for p-adics.

If nobody else has input, I can go ahead and make this change.

comment:16 in reply to: ↑ 14 Changed 4 years ago by roed

Replying to caruso:

IMO, It's a kinda weird to promote reduction modulo p to a coercion map but not reduction modulo pn.

I agree that it's kind of weird. I'd really like for them both to be coercions, but I explain above why that doesn't seem to be possible.

I think it's probably okay to also delete the coercion to the residue field. For integers, you really want to be able to ask for a+1 if a is an element of Zmod(N), but I think that this is a lot less important for p-adics.

comment:17 Changed 4 years ago by jpflori

Can we try to live with a conversion? if that raise problems we cna try to make all maps coercions. But as David said, there is a precendent with other constructions in sagE.

comment:18 Changed 4 years ago by git

  • Commit changed from c08c76432d957b62d799581051cbab40c9766de2 to 19d02443906f72b3cd0f911e8df9de11f98c1628

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

19d0244Merge branch 'u/roed/residue_coercion' of git://trac.sagemath.org/sage into t/8240/residue_coercion

comment:19 Changed 4 years ago by git

  • Commit changed from 19d02443906f72b3cd0f911e8df9de11f98c1628 to 48d259dca39e9870be6c33fef6b6d5806ddfc3e7

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

6efed0bMerge branch 'u/roed/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number
f80ca76Working on adding iterators
030251cRestructing of p-adic expansions
3c912f9Adding documentation and making small changes to names
7e037b3cleaning up a couple of hyperelliptic curve changes
acf6b66Fix implementation in linear_code that used padded_list
1f8aa2eFix NOTES
5cf279bFix izip
d7d5fb6Fixing documentation
48d259dMerge branch 't/14825/polynomial_representation_of_a_padic_number' into t/8240/residue_coercion

comment:20 Changed 4 years ago by roed

  • Dependencies set to #14825

comment:21 Changed 4 years ago by git

  • Commit changed from 48d259dca39e9870be6c33fef6b6d5806ddfc3e7 to fa7fb849cfb44527011b7bc5b9c45d3b990defa1

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

fa7fb84Remove coercions to residue field, change _element_constructor to element_class, update docstrings

comment:22 Changed 4 years ago by git

  • Commit changed from fa7fb849cfb44527011b7bc5b9c45d3b990defa1 to 6daea341d6b3247f5425ad453562d02c7c89826a

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

6daea34Fix doctests

comment:23 Changed 4 years ago by git

  • Commit changed from 6daea341d6b3247f5425ad453562d02c7c89826a to 7abedbf9507335b76e3990aedf852b27c84f3960

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

7abedbfFix finite field coercion doctest

comment:24 Changed 4 years ago by git

  • Commit changed from 7abedbf9507335b76e3990aedf852b27c84f3960 to 2e38829ddb42f5d19cde07cd4aa8582d82ab03e1

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

46f9caeMerge branch 'develop' into t/14825/polynomial_representation_of_a_padic_number
e51c0f5Merge branch 'u/roed/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number
b6457b1Moving SEEALSO to the end of the docstring
b81b722Remove use of depraceted list()
04a1579Fix NOTES blocks
2e38829Merge branch 't/14825/polynomial_representation_of_a_padic_number' into t/8240/residue_coercion

Changed 4 years ago by roed

Diff against #14825 for ease of review

comment:25 Changed 4 years ago by git

  • Commit changed from 2e38829ddb42f5d19cde07cd4aa8582d82ab03e1 to 3a0ff1b42f2e0d29059af91a8043ec6e655c7f50

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

3a0ff1bFix RST problem

comment:26 Changed 4 years ago by roed

I'm getting an error from the docbuild plugin:

OSError: [padics   ] ...docstring of sage.rings.padics.padic_generic.pAdicGeneric.residue_ring:1: WARNING: Inline interpreted text or phrase reference start-string without end-string.

The relevant docstring is

    """
    Return the quotient of the ring of integers by the nth power of its maximal ideal.

    EXAMPLES::

        sage: S.<x> = ZZ[]
 	sage: W.<w> = Zp(5).extension(x^2 - 5)
 	sage: W.residue_ring(1)
 	Ring of integers modulo 5

    The following requires implementing more general Artinian rings::

 	sage: W.residue_ring(2)
 	Traceback (most recent call last):
 	...
 	NotImplementedError
    """

Any ideas what might be going on?

comment:27 Changed 4 years ago by chapoton

in Returns the quotient of the ring of integers by the `n`th power of the maximal ideal. add a space after `n`

comment:28 Changed 4 years ago by chapoton

in

richcmp((type(self), self.domain(), self.codomain()), (type(other), other.domain(), other.codomain()), op)

you compare types, which is not allowed in python3.

EDIT Instead you can do something like

if type(self) != type(other):
    return NotImplemented
return richcmp(...)
Last edited 4 years ago by chapoton (previous) (diff)

comment:29 Changed 4 years ago by git

  • Commit changed from 3a0ff1b42f2e0d29059af91a8043ec6e655c7f50 to a9b23a931959297810de921c1a132e6a6901d7c5

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

a9b23a9Fixing richcmp for python 3 compatibility

comment:30 Changed 4 years ago by roed

Thanks; I've fixed the richcmp error you pointed out.

comment:31 Changed 4 years ago by git

  • Commit changed from a9b23a931959297810de921c1a132e6a6901d7c5 to 9b0b60fe528ad04d136590e85211e11a75ce0c1d

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

9b0b60fTry adding a - after

comment:32 Changed 4 years ago by git

  • Commit changed from 9b0b60fe528ad04d136590e85211e11a75ce0c1d to d625912ad7cf2e23ac44ba2251219e87e98f0841

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

d625912Fix docstring of residue_ring

comment:33 Changed 4 years ago by roed

Tests pass.

comment:34 Changed 4 years ago by kedlaya

I confess to not having inspected all of this code line-by-line. But I did go through the main blocks, and I tried a few tests of my own without finding any issues. Positive review.

comment:35 Changed 4 years ago by kedlaya

  • Reviewers set to Kiran Kedlaya
  • Status changed from needs_review to positive_review

comment:36 Changed 4 years ago by tscrim

  • Milestone set to sage-8.1

comment:37 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:38 Changed 4 years ago by saraedum

  • Branch changed from u/roed/residue_coercion to u/saraedum/residue_coercion

comment:39 Changed 4 years ago by saraedum

  • Commit changed from d625912ad7cf2e23ac44ba2251219e87e98f0841 to ff3d5d771e9ae100d5af264399874c2d53be5091
  • Status changed from needs_work to positive_review

No conflicts for me.


New commits:

ff3d5d7Merge remote-tracking branch 'trac/develop' into t/8240/residue_coercion

comment:40 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

wait for next beta

comment:41 Changed 4 years ago by roed

  • Branch changed from u/saraedum/residue_coercion to u/roed/residue_coercion

comment:42 Changed 4 years ago by roed

  • Commit changed from ff3d5d771e9ae100d5af264399874c2d53be5091 to 1fc48d1a6f4baf9625c291fdb8eae0f8d1109d3c
  • Status changed from needs_work to positive_review

Fixed the merge conflict and ran all tests.


New commits:

1fc48d1Merge commit 'd625912ad7' into t/8240/residue_coercion

comment:43 Changed 4 years ago by vbraun

  • Branch changed from u/roed/residue_coercion to 1fc48d1a6f4baf9625c291fdb8eae0f8d1109d3c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.