change for p-adic rings

[caruso]

Started as a proposal to change precision of rings, this is now a more general change method.

[caruso]

As discussed in Sage Days 71, I tried to implement this method using the construction functor but it seems that the latter is only defined for Zp or Qp but not for extensions. So, what should I do? Implement the methods construction as well? Something else?

[roed]

Implementing construction is a good idea anyway, since they're used in coercion. And until we change extensions to be defined by exact polynomials, you'll need to raise the precision of the base ring as well, so it's probably necessary.

---

New commits:

​9ca8df8

Generic function change_precision

[git]

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

​e16f79f	Add construction method for extensions of p-adic rings and fields, switch implementation of fraction_field and integer_ring
​a88b630	Fixing problems revealed by tests
​fdd03cb	Merge branch 'u/roed/padic-floats' of git://trac.sagemath.org/sage into t/20310/change_precision
​53d2446	Added ability to edit show_prec to p-adic factory

[git]

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

​55c335e

Fixing doctest problems in the new change method for p-adic rings/fields

[git]

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

​fcc8aa5

Fixing various problems with the new change method

[git]

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

​3b2d94a

Remove old code for fraction_field and integer_ring from padic_extension_generic

[git]

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

​f386dc6

Fix bug in ring_of_integers

[caruso]

I'm a bit sceptical about the possibility of changing p (and even q). Can you show me a situation where this can be needed?

Adding show_prec is nice but is it really the subject of this ticket?

Doctests implying the keyword base are missing. The same is true for show_prec. More generally, I suggest to add many more doctests ensuring that your code works in a wide range of situations (for ramified, unramified extensions, for extensions constructing via CompletionFunction and AlgebraicExtensionFunctor, etc.)

[saraedum]

Xavier: Thanks for having a look at this as well. We discussed the changing of p and q briefly and actually I find it very useful. Often I am only really interested in the ramified part of a tower of extensions (on my fork that already does more general towers.) But for some computation to work out, I need a certain unramified element at the bottom, so I don't want to manually reconstruct the whole tower, so raising and lowering q is quite useful. Changing p is mostly interesting in general extensions where you want to see how a certain polynomial behaves wrt to different primes. For the Eisenstein extensions that we support atm it is not too useful.

I think it's Ok to put the show_prec in here, I'm fine with reviewing it at least :)

I agree that doctests for base and show_prec should be added. However, I think that in general not all possible cases should be covered by explicit doctests. It usually makes the documentation overwhelming. If we want to have coverage, we should write unit tests for that instead. That's btw one of the topics that we want to push in Burlington! Imho, the testing for change is Ok except for the untested keywords.

[saraedum]

David: should get_key_base set show_prec? Otherwise, show_prec=None and show_prec=True/False produces two non-identical parents for the same ring I think.

[git]

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

​f4a5345

Move show prec logic into get_key

[roed]

I fixed the show_prec issue.

[saraedum]

Looks good to me. Feel free to set this to positive review if the tests pass.

[caruso]

sage: R = Zp(5)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-1247c7c70bf9> in <module>()
----> 1 R = Zp(Integer(5))

/home/xavier/sage/src/sage/structure/factory.pyx in sage.structure.factory.UniqueFactory.__call__ (/home/xavier/sage/src/build/cythonized/sage/structure/factory.c:1728)()
    360             False
    361         """
--> 362         key, kwds = self.create_key_and_extra_args(*args, **kwds)
    363         version = self.get_version(sage_version)
    364         return self.get_object(version, key, kwds)

/home/xavier/sage/src/sage/structure/factory.pyx in sage.structure.factory.UniqueFactory.create_key_and_extra_args (/home/xavier/sage/src/build/cythonized/sage/structure/factory.c:2921)()
    458             ((3, ('x',), None, 'modn', "{'foo': 'value'}", 3, 1, True), {'foo': 'value'})
    459         """
--> 460         return self.create_key(*args, **kwds), {}
    461 
    462     def create_key(self, *args, **kwds):

/home/xavier/sage/local/lib/python2.7/site-packages/sage/rings/padics/factory.pyc in create_key(self, p, prec, type, print_mode, halt, names, ram_name, print_pos, print_sep, print_alphabet, print_max_terms, show_prec, check)
   1608         """
   1609         return get_key_base(p, prec, type, print_mode, halt, names, ram_name, print_pos, print_sep, print_alphabet,
-> 1610                             print_max_terms, show_prec, check, ['capped-rel', 'fixed-mod', 'capped-abs', 'floating-point'])
   1611 
   1612     def create_object(self, version, key):

/home/xavier/sage/local/lib/python2.7/site-packages/sage/rings/padics/factory.pyc in get_key_base(p, prec, type, print_mode, halt, names, ram_name, print_pos, print_sep, print_alphabet, print_max_terms, show_prec, check, valid_non_lazy_types)
    161         if type == 'floating-point':
    162             show_prec = False
--> 163         elif print_mode ('series', 'terse', 'val-unit'):
    164             show_prec = True
    165         else:

TypeError: 'str' object is not callable

[caruso]

By the way, I guess that your changes is not compatible with ticket #22103.

It's of course not that annoying (and, as I've already said, I'm fine with adding the keyword show_prec) but maybe we could have a discussion about the "right way" to print p-adic numbers.

[git]

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

​e376b90

Fix doctest errors and a bug in change related to show_prec

[roed]

I just fixed various doctest issues and added some code for the case of changing print_mode (show_prec was not changed). I ran all tests on the result, and they all pass, but I think I need someone to take a look rather than just setting it to positive review.

[caruso]

Well, I'm still not that convinced by changing p. You say that "it is mostly interesting in general extensions where you want to see how a certain polynomial behaves wrt to different primes" but I guess that the modulus is just stored modulo p^prec, so converting it in others Z_p's may lead to very unexpected behaviours, no? I nevertheless agree that changing q (keeping the same p) makes more sense.

Anyway, changing p actually does not work in many cases:

sage: R.<a> = Zq(5^3)
sage: R.change(p=7)
...
NotImplementedError: conversion between padic extensions not implemented

For ramified extensions, something very stange happens:

sage: R.<a> = Zp(5).extension(x^3 + 5)
sage: R
Eisenstein Extension of 5-adic Ring with capped relative precision 20 in a defined by  (1 + O(5^20))*x^3 + (O(5^21))*x^2 + (O(5^21))*x + (5 + O(5^21))

sage: S = R.change(p=7)
sage: S
Unramified Extension of 7-adic Ring with capped relative precision 20 in a defined by (1 + O(5^20))*x^3 + (O(5^20))*x^2 + (O(5^20))*x + (5 + O(5^20))

Look at the modulus, it is still written with some O(5^20)'s (whereas we are supposed to be on a 7-adic ring). Even more stranger:

sage: c = S.modulus()[0]; c
5 + O(5^20)
sage: c.parent()
7-adic Ring with capped relative precision 20

sage: c.parent() is Zp(7)
False

As a conclusion, I would be in favour to remove this feature.

[caruso]

Other issues with the change of precision:

sage: R.<a> = Zq(5^3)
sage: S = R.change(prec=50)
sage: S
Unramified Extension of 5-adic Ring with capped relative precision 50 in a defined by (1 + O(5^20))*x^3 + (O(5^20))*x^2 + (3 + O(5^20))*x + (3 + O(5^20))

The defining polynomial has not been lifted to precision 50.

In fact, it seems that we cannot compute at all at precision 50 is S:

sage: S.gen()
a + O(5^20)
sage: S.precision_cap()
20

For ramified extensions, the precision on the modulus decreases:

sage: R.<a> = Zp(5).extension(x^3 + 5)
sage: R
Eisenstein Extension of 5-adic Ring with capped relative precision 20 in a defined by (1 + O(5^20))*x^3 + (O(5^21))*x^2 + (O(5^21))*x + (5 + O(5^21))
sage: S = R.change(prec=50)
sage: S
Eisenstein Extension of 5-adic Ring with capped relative precision 20 in a defined by (1 + O(5^18))*x^3 + (O(5^18))*x^2 + (O(5^18))*x + (5 + O(5^18))

but (unexpectedly) computations in S seem to be fine.

[roed]

Good points. We've also been thinking about switching defining polynomials to being exact, in which case some of the precision issues go away.

I'll take a look at this when I'm back, in a week and a half or so.

[git]

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

​543088a

Fix broken printing when changing p in a p-adic extension, raise error when creating an extension with precision larger than the precision of the defining polynomial

[git]

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

​b643d24

Delay substitution of DEFAULT_PREC in padics factory

[git]

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

​e935ca9

Change Zq and Qq to be defined by an exact polynomial

[roed]

I fixed the printing problem that was causing the weird behavior you observed when changing p. If the defining polynomial is defined over the integers, then changing p works; if not it doesn't.

I also added an error if you try to increase the precision above the limit imposed by the defining polynomial.

[git]

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

​547b66d

Fix error in check=False constructor for Qq

[roed]

All tests pass for me.

[caruso]

Not a complete review, but I noticed the following:

In the doctest of the method change, some items are introduced by -- instead of -.

When creating an unramified extension, we cannot specify the variable name using the keyword unram_name:

sage: R = Zp(5)
sage: R.change(q=25, unram_name='a')
...
TypeError: You must specify the name of the generator

sage: R2.<a> = Zq(5^2)
sage: R2.change(q=125,unram_name='b')
Unramified Extension of 5-adic Ring with capped relative precision 20 in a defined by (1 + O(5^20))*x^3 + (O(5^20))*x^2 + (3 + O(5^20))*x + (3 + O(5^20))

(Note nevertheless than using names instead of unram_name works fine.)

I am a bit lost with exact VS inexact defining polynomials. For instance:

sage: var('x')
x
sage: R.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R.change(p=11)
Unramified Extension of 11-adic Ring with capped relative precision 20 in a defined by (1 + O(11^20))*x^2 + (2 + O(11^20))*x + (4 + O(11^20))

sage: ZX.<x> = ZZ[]
sage: R.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R.change(p=11)
...
NotImplementedError: conversion between padic extensions not implemented

By the way, even when the defining polynomial is exact, the methods modulus and defining_polynomial return an inexact polynomial. Is there a method that returns the exact defining polynomial (i.e. the content of the attribute _pre_poly as far as I understand)?

[git]

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

​bf8d760

Fix formatting errors in change docstring, allow specifying unram_name rather than names in changing q on Zp or Qp

[roed]

Replying to caruso:

Not a complete review, but I noticed the following:

In the doctest of the method change, some items are introduced by -- instead of -.

Fixed, thanks.

When creating an unramified extension, we cannot specify the variable name using the keyword unram_name:

sage: R = Zp(5)
sage: R.change(q=25, unram_name='a')
...
TypeError: You must specify the name of the generator

sage: R2.<a> = Zq(5^2)
sage: R2.change(q=125,unram_name='b')
Unramified Extension of 5-adic Ring with capped relative precision 20 in a defined by (1 + O(5^20))*x^3 + (O(5^20))*x^2 + (3 + O(5^20))*x + (3 + O(5^20))

(Note nevertheless than using names instead of unram_name works fine.)

Fixed.

I am a bit lost with exact VS inexact defining polynomials. For instance:

sage: var('x')
x
sage: R.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R.change(p=11)
Unramified Extension of 11-adic Ring with capped relative precision 20 in a defined by (1 + O(11^20))*x^2 + (2 + O(11^20))*x + (4 + O(11^20))

sage: ZX.<x> = ZZ[]
sage: R.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R.change(p=11)
...
NotImplementedError: conversion between padic extensions not implemented

I get that both of these work. It's possible that your cache has been poisoned by trying to first create R using a p-adic modulus, and then later with a modulus over Z? This is the kind of thing that might be alleviated by #23331.

By the way, even when the defining polynomial is exact, the methods modulus and defining_polynomial return an inexact polynomial. Is there a method that returns the exact defining polynomial (i.e. the content of the attribute _pre_poly as far as I understand)?

I don't think there's currently a method to get _pre_poly. I would suggest holding off on that change until #23331.

[caruso]

I get that both of these work. It's possible that your cache has been poisoned by trying to first create R using a p-adic modulus, and then later with a modulus over Z? This is the kind of thing that might be alleviated by #23331.

Good point. With a fresh session of sage, I have:

sage: R.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R.change(p=11)
Unramified Extension of 11-adic Ring with capped relative precision 20 in a defined by (1 + O(11^20))*x^2 + (2 + O(11^20))*x + (4 + O(11^20))
sage: 
sage: ZX.<x> = ZZ[]
sage: R.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R.change(p=11)
Unramified Extension of 11-adic Ring with capped relative precision 20 in a defined by (1 + O(11^20))*x^2 + (2 + O(11^20))*x + (4 + O(11^20))

but:

sage: ZpX.<x> = Zp(5)[]
sage: R1.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R1.change(p=11)
...
NotImplementedError: conversion between padic extensions not implemented

sage: ZX.<x> = ZZ[]
sage: R2.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R2.change(p=11)
...
NotImplementedError: conversion between padic extensions not implemented

and conversely:

sage: ZX.<x> = ZZ[]
sage: R2.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R2.change(p=11)
Unramified Extension of 11-adic Ring with capped relative precision 20 in a defined by (1 + O(11^20))*x^2 + (2 + O(11^20))*x + (4 + O(11^20))

sage: ZpX.<x> = Zp(5)[]
sage: R1.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R1.change(p=11)
Unramified Extension of 11-adic Ring with capped relative precision 20 in a defined by (1 + O(11^20))*x^2 + (2 + O(11^20))*x + (4 + O(11^20))

I guess that the problem comes from the factory because, in the above examples, R1 is the same as R2 (i.e. R1 is R2 answers True).

[saraedum]

Unless Xavier has any objections of course.

[caruso]

Well, I am a bit worried to give a positive review after comment 35 (which is clearly a bug for me).

I am fine with fixing this in ticket #23331 but, in that case, I would wait for #23331 to be written and positively reviewed before giving a positive review to the current ticket (#20310)

[saraedum]

I don't see how this is related to this ticket really. This is just a bug in the factory that has always been there. The same problem at least happens on a stable sage

sage: ZpX.<x> = Zp(5)[]
sage: R1.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: ZX.<x> = ZZ[]
sage: R2.<a> = Zq(5^2, modulus=x^2+2*x+4)
sage: R1 is R2
True

I guess my point is that you could use this to construct problems with many methods in Sage. I do not see why we should have change() be blocked by this now. (We should of course fix this, nevertheless…)

[caruso]

I do not agree. This was not a bug until this ticket implemented a method, namely change, that decides to treat differently extensions defined by exact and inexact polynomials.

[saraedum]

Ok. I guess you're right that this was much more unlikely to cause trouble before.

I still don't think that this should block this ticket, but I guess we have to wait for #23331 then.

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​7e21e77	Adding an exact modulus to p-adic rings and fields, changing printing for p-adic extensions, removing some vestigal lazy p-adic code
​dbbc631	Fix pickles
​6cb9d22	Remove extraneous newlines in padic_ZZ_pX_FM_element.pyx
​daf51fa	Merge branch 'u/roed/change_precision' of git://trac.sagemath.org/sage into t/20310/change_precision
​8351aa9	Fixing doctest errors from merge

[roed]

I merged in the dependency #23331; all tests pass.

[git]

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

​4ac09ac	Fix changing print mode to digits for eisenstein extensions
​85019cc	Fix errors due to moving digits code earlier in the change function

[git]

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
​39043f1	Merge branch 't/23331/allow_exact_defining_polynomials_for_p_adic_extensions' into t/20310/change_precision

[saraedum]

positive review once the patchbot is happy.

---

New commits:

​77779ea	minor docstring changes
​6e2495f	Merge branch 't/23331/allow_exact_defining_polynomials_for_p_adic_extensions' into t/20310/change_precision
​7428353	minor docstring fixes

[git]

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

​1754b44

Fix exact=True errors in the right branch

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​6ba62dd	Fix SEEALSO again
​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
​561f5ac	Fix doctest errors
​3142701	Merge branch 'u/roed/change_precision' of git://trac.sagemath.org/sage into t/20310/change_precision

[roed]

I just merged in the updated #23331.

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​138d939

Fix string representation doctest from #22103

[roed]

Diff against 23331

[roed]

For ease of reviewing, I've attached a diff against #23331.

[vbraun]

PDF docs don't build

[git]

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

​b18929a

Merge branch 'develop' into t/20310/change_precision

[git]

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

​86b58de

Fix latex error

[roed]

Alright, make succeeds at building the docs now.

[saraedum]

daucher still reports a failing docbuild. I guess that's because I told it not to clean the docs when rebuilding.