#14825 closed enhancement (fixed)
Iterators for padic expansions, polynomial representations of padic elements
Reported by:  caruso  Owned by:  roed 

Priority:  major  Milestone:  sage6.4 
Component:  padics  Keywords:  sd87, days88, polynomial representation 
Cc:  saraedum  Merged in:  
Authors:  Xavier Caruso, David Roe, Julian Rüth  Reviewers:  Julian Rüth, David Roe 
Report Upstream:  N/A  Work issues:  plugin error 
Branch:  04a1579 (Commits)  Commit:  
Dependencies:  #20310  Stopgaps: 
Description (last modified by )
This method makes a few additions/changes to padics.
 If
K
is an extension ofZ_p
whose generator isa
, we add a methodpolynomial
which takes as input an elementx
inK
and outputs a polynomialP
with coefficients inZ_p
such thatP(a) = x
.
sage: K.<a> = Qq(5^3) sage: a.polynomial() (1 + O(5^20))*x + (O(5^20))
 Rename the
list()
method toexpansion()
and have it return a custom iterable instead of a list  Rename the
teichmuller_list()
method toteichmuller_expansion()
and have it return a custom iterable instead of a list  Add an optional argument
n
toexpansion()
andteichmuller_expansion()
, providing a single digit in the padic expansion.  Fix inconsistencies in
teichmuller_expansion()
for different precision types  Deprecate
padded_list
since this functionality is available either fromexpansion
or fromInteger.digits
using thepadto
keyword.  Copy sections of inclusions
ZZ>Zp
when they’re used within the coercion system
Attachments (2)
Change History (67)
Changed 7 years ago by
comment:1 Changed 7 years ago by
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 7 years ago by
 Status changed from needs_review to needs_work
comment:5 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:6 Changed 3 years ago by
 Branch set to u/roed/polynomial_representation_of_a_padic_number
comment:7 Changed 3 years ago by
 Commit set to 4088f9b3fa1033e77b3edf935c3608ebc806d5e3
Branch pushed to git repo; I updated commit sha1. New commits:
4088f9b  Fixing doctest errors, changes to ZZ_pX padic types

comment:8 Changed 3 years ago by
 Cc saraedum added
I got a little carried away with some other changes. In particular, I renamed and deprecated list and teichmuller list. There are still two failing doctests, but I'm going to be offline for a week or so and wanted to push my changes.
comment:9 Changed 3 years ago by
 Commit changed from 4088f9b3fa1033e77b3edf935c3608ebc806d5e3 to 24ee4fff0f114df6cc104a95b81673477c1294b0
Branch pushed to git repo; I updated commit sha1. New commits:
24ee4ff  Fixing doctest errors due to switching from list to expansion

comment:10 Changed 3 years ago by
 Commit changed from 24ee4fff0f114df6cc104a95b81673477c1294b0 to cbfeb3429c52b82163b7abc747abba80d9b2f138
Branch pushed to git repo; I updated commit sha1. New commits:
cbfeb34  Making behavior of teichmuller_expansion uniform across precision types, adding doctests

comment:11 Changed 3 years ago by
 Commit changed from cbfeb3429c52b82163b7abc747abba80d9b2f138 to f5e24404b970f6b4e674643a10f442c9257e0a63
Branch pushed to git repo; I updated commit sha1. New commits:
f5e2440  Fix 'x'>'var' typo in some docstrings, add polynomial method to ntl padic types

comment:12 Changed 3 years ago by
 Commit changed from f5e24404b970f6b4e674643a10f442c9257e0a63 to 779ddb8fa159e88e3a53ba7d3fe786fed815afc5
Branch pushed to git repo; I updated commit sha1. New commits:
779ddb8  Remove added NOTES: in CR_template

comment:13 Changed 3 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
 Summary changed from Polynomial representation of a padic number to Polynomial representations of padic elements, renaming list and teichmuller_list
comment:14 Changed 3 years ago by
By the way, I ran all tests and they pass.
comment:15 Changed 3 years ago by
 Branch changed from u/roed/polynomial_representation_of_a_padic_number to u/saraedum/polynomial_representation_of_a_padic_number
comment:16 Changed 3 years ago by
 Commit changed from 779ddb8fa159e88e3a53ba7d3fe786fed815afc5 to 985a23a4332d6d8b6ccf7f104f87b634903e3b60
David, feel free to set this to positive review. My only question would be whether polynomial
should be added to API.pxi
?
New commits:
985a23a  amend documentation of ccoefficients

comment:17 Changed 3 years ago by
 Status changed from needs_review to positive_review
Misunderstood something about polynomial. Never mind.
comment:18 Changed 3 years ago by
 Branch changed from u/saraedum/polynomial_representation_of_a_padic_number to u/roed/polynomial_representation_of_a_padic_number
comment:19 Changed 3 years ago by
 Commit changed from 985a23a4332d6d8b6ccf7f104f87b634903e3b60 to 3cddbdd8e66606f5d4f8c225316a2b3285142181
 Status changed from positive_review to needs_review
New commits:
3cddbdd  Fix doctests

comment:20 Changed 3 years ago by
 Reviewers set to Julian Rüth
 Status changed from needs_review to positive_review
comment:21 Changed 3 years ago by
 Status changed from positive_review to needs_work
sage: K.<a> = Qq(7^3,4) sage: b = (a+1)/7 sage: b.polynomial() sig_error() without sig_on() <BOUM>
comment:22 Changed 3 years ago by
 Commit changed from 3cddbdd8e66606f5d4f8c225316a2b3285142181 to 1ec63e7cf496c866fb7fd7ede7c7310844aac877
Branch pushed to git repo; I updated commit sha1. New commits:
1ec63e7  Merge branch 'u/roed/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number

comment:23 Changed 3 years ago by
 Commit changed from 1ec63e7cf496c866fb7fd7ede7c7310844aac877 to b9c2fe42e7e93e64a03dbe13bd792988f38fa735
comment:24 Changed 3 years ago by
 Status changed from needs_work to needs_review
I fixed the segfault, and tests pass.
comment:25 Changed 3 years ago by
 Commit changed from b9c2fe42e7e93e64a03dbe13bd792988f38fa735 to 1eeb367a038d5752ff793ccb71094af497d75b37
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
85019cc  Fix errors due to moving digits code earlier in the change function

39043f1  Merge branch 't/23331/allow_exact_defining_polynomials_for_p_adic_extensions' into t/20310/change_precision

6e2495f  Merge branch 't/23331/allow_exact_defining_polynomials_for_p_adic_extensions' into t/20310/change_precision

7428353  minor docstring fixes

99dccf6  Merge branch 't/23331/allow_exact_defining_polynomials_for_p_adic_extensions' into t/20310/change_precision

ef4ed33  Fix SEEALSO:

1754b44  Fix exact=True errors in the right branch

3142701  Merge branch 'u/roed/change_precision' of git://trac.sagemath.org/sage into t/20310/change_precision

138d939  Fix string representation doctest from #22103

1eeb367  Merge branch 't/20310/change_precision' into t/14825/polynomial_representation_of_a_padic_number

comment:26 Changed 3 years ago by
 Dependencies set to #20310
Adding a dependency for merging purposes.
comment:27 Changed 3 years ago by
For ease of reviewing, I've attached a diff against #20310.
comment:28 Changed 3 years ago by
 Branch changed from u/roed/polynomial_representation_of_a_padic_number to u/saraedum/polynomial_representation_of_a_padic_number
comment:29 followup: ↓ 35 Changed 3 years ago by
 Branch changed from u/saraedum/polynomial_representation_of_a_padic_number to u/roed/polynomial_representation_of_a_padic_number
 Status changed from needs_review to needs_info
Ok. Looks good. I fixed a few very minor documentation issues. I also wrote a unit test for expansion and it fails for some instances. This is likely only a error in the documentation:
sage: K=Qp(2) sage: K(2).expansion() [1] sage: K(2).expansion(n=0) 0 # should return 1? sage: K(2).expansion(n=1) 1 # sould return 0?
That's at least how I understand the expansion() docstring. Is the current behaviour intended?
comment:30 followup: ↓ 34 Changed 3 years ago by
Here is something that the unit test found that seems to be a bug:
sage: ZpCA(2)(2).expansion(lift_mode='teichmuller') [1 + O(2^19)] sage: ZpFM(2)(2).expansion(lift_mode='teichmuller') [1 + O(2^20)]
comment:31 Changed 3 years ago by
 Work issues set to failing doctests
comment:32 Changed 3 years ago by
comment:33 Changed 3 years ago by
 Branch changed from u/roed/polynomial_representation_of_a_padic_number to u/saraedum/polynomial_representation_of_a_padic_number
comment:34 in reply to: ↑ 30 ; followup: ↓ 40 Changed 3 years ago by
 Commit changed from 1eeb367a038d5752ff793ccb71094af497d75b37 to 6732d3814afa9650b29356ffa2b2d1f225e0b343
Replying to saraedum:
Here is something that the unit test found that seems to be a bug:
sage: ZpCA(2)(2).expansion(lift_mode='teichmuller') [1 + O(2^19)] sage: ZpFM(2)(2).expansion(lift_mode='teichmuller') [1 + O(2^20)]
Why is this a bug?
New commits:
1e07709  fix typo

ec9f8e6  fix typo

dfa2a28  fix typo

1bb288f  protect LaTeX commands

054e5d6  clarify where _list_zero comes from

0b7fd02  default docstring layout

def3897  replace p with pi and clarify meaning of expansion

99c40d6  add unit test for expansion

6732d38  coefficients might be lists in the maximal unramified subextension

comment:35 in reply to: ↑ 29 ; followup: ↓ 38 Changed 3 years ago by
Replying to saraedum:
Ok. Looks good. I fixed a few very minor documentation issues. I also wrote a unit test for expansion and it fails for some instances. This is likely only a error in the documentation:
sage: K=Qp(2) sage: K(2).expansion() [1] sage: K(2).expansion(n=0) 0 # should return 1? sage: K(2).expansion(n=1) 1 # should return 0?That's at least how I understand the expansion() docstring. Is the current behaviour intended?
Yeah, this is intended. When you pass in a value for n
it gives you the coefficient of p^{n}, rather than the n
th term in the list. I've updated the test (but haven't pushed yet).
comment:36 followup: ↓ 39 Changed 3 years ago by
So, your test causes padic_expansion_generic.py
to time out, since it's running with precision cap 10000. One way to improve the situation might be to change expansion to return an iterator (and then change _test_expansion
to top out at testing at most 40 terms or something). This ticket might be a good time to do this, since we're already deprecating list; it's not hard to change the behavior of expansion
at the same time (which we're in fact doing by adding the n
keyword).
What do you think?
comment:37 Changed 3 years ago by
 Branch changed from u/saraedum/polynomial_representation_of_a_padic_number to u/roed/polynomial_representation_of_a_padic_number
comment:38 in reply to: ↑ 35 Changed 3 years ago by
 Commit changed from 6732d3814afa9650b29356ffa2b2d1f225e0b343 to 40737f6ae9eafcec67129892b5c5df591023ab85
Replying to roed:
Replying to saraedum:
Ok. Looks good. I fixed a few very minor documentation issues. I also wrote a unit test for expansion and it fails for some instances. This is likely only a error in the documentation:
sage: K=Qp(2) sage: K(2).expansion() [1] sage: K(2).expansion(n=0) 0 # should return 1? sage: K(2).expansion(n=1) 1 # should return 0?That's at least how I understand the expansion() docstring. Is the current behaviour intended?
Yeah, this is intended. When you pass in a value for
n
it gives you the coefficient of p^{n}, rather than then
th term in the list. I've updated the test (but haven't pushed yet).
Ok. Then the documentation should be more explicit. I thought it's just a shortcut to get the nth element in the list.
New commits:
0d46568  Merge branch 'develop' into t/14825/polynomial_representation_of_a_padic_number

acc606a  Merge branch 'u/saraedum/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number

40737f6  Fix some of the errors in _test_expansion

comment:39 in reply to: ↑ 36 Changed 3 years ago by
Replying to roed:
So, your test causes
padic_expansion_generic.py
to time out, since it's running with precision cap 10000. One way to improve the situation might be to change expansion to return an iterator (and then change_test_expansion
to top out at testing at most 40 terms or something). This ticket might be a good time to do this, since we're already deprecating list; it's not hard to change the behavior ofexpansion
at the same time (which we're in fact doing by adding then
keyword).What do you think?
Sounds good.
comment:40 in reply to: ↑ 34 Changed 3 years ago by
Replying to roed:
Replying to saraedum:
Here is something that the unit test found that seems to be a bug:
sage: ZpCA(2)(2).expansion(lift_mode='teichmuller') [1 + O(2^19)] sage: ZpFM(2)(2).expansion(lift_mode='teichmuller') [1 + O(2^20)]Why is this a bug?
The docstring says that (since this is not a field) summing these with powers of pi should return the original element, i.e., (1 + O(2^19))*2^0 == 2
.
comment:41 Changed 3 years ago by
 Commit changed from 40737f6ae9eafcec67129892b5c5df591023ab85 to 6efed0be21d740b73063e0c606a95b0cd73f3034
Branch pushed to git repo; I updated commit sha1. New commits:
6efed0b  Merge branch 'u/roed/polynomial_representation_of_a_padic_number' of git://trac.sagemath.org/sage into t/14825/polynomial_representation_of_a_padic_number

comment:42 Changed 3 years ago by
 Keywords sd87 days88 added
 Status changed from needs_info to needs_work
comment:43 Changed 3 years ago by
 Commit changed from 6efed0be21d740b73063e0c606a95b0cd73f3034 to f80ca7607e136ba5989ff30318f7c7a681317081
Branch pushed to git repo; I updated commit sha1. New commits:
f80ca76  Working on adding iterators

comment:44 Changed 3 years ago by
 Commit changed from f80ca7607e136ba5989ff30318f7c7a681317081 to 030251c4e8ce05066a7fcd6618078f6d8b537f14
Branch pushed to git repo; I updated commit sha1. New commits:
030251c  Restructing of padic expansions

comment:45 Changed 3 years ago by
 Commit changed from 030251c4e8ce05066a7fcd6618078f6d8b537f14 to 3c912f948e9282a780e34ddfc2ddf4ae72f69678
Branch pushed to git repo; I updated commit sha1. New commits:
3c912f9  Adding documentation and making small changes to names

comment:46 Changed 3 years ago by
 Commit changed from 3c912f948e9282a780e34ddfc2ddf4ae72f69678 to 7e037b337e3d5057fb0734e81030396fe814d0d4
Branch pushed to git repo; I updated commit sha1. New commits:
7e037b3  cleaning up a couple of hyperelliptic curve changes

comment:47 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:48 Changed 3 years ago by
 Commit changed from 7e037b337e3d5057fb0734e81030396fe814d0d4 to acf6b666b144d0b98b16383fa47cfa9133019bd8
Branch pushed to git repo; I updated commit sha1. New commits:
acf6b66  Fix implementation in linear_code that used padded_list

comment:49 Changed 3 years ago by
 Description modified (diff)
 Summary changed from Polynomial representations of padic elements, renaming list and teichmuller_list to Iterators for padic expansions, polynomial representations of padic elements
comment:50 Changed 3 years ago by
 Work issues changed from failing doctests to plugin failures
comment:51 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:52 followup: ↓ 54 Changed 3 years ago by
 Work issues changed from plugin failures to plugin failures, patchbot happy ⇒ positive_review
This looks very good. Is there a follow up ticket to implement the new __getitem__
?
comment:53 Changed 3 years ago by
 Commit changed from acf6b666b144d0b98b16383fa47cfa9133019bd8 to d7d5fb612c45ed3e51724f3834d194415cc0a122
comment:54 in reply to: ↑ 52 Changed 3 years ago by
comment:55 Changed 3 years ago by
 Status changed from needs_work to needs_review
 Work issues plugin failures, patchbot happy ⇒ positive_review deleted
Patchbot complains about .. SEEALSO::
, but I'm confused: this is the correct syntax described both in the developer guide and the patchbot source code! Am I missing something?
The doctest errors are the normal glpk issues, which aren't due to this ticket.
comment:56 Changed 3 years ago by
 Work issues set to merge conflicts, plugin error
comment:57 Changed 3 years ago by
 Branch changed from u/roed/polynomial_representation_of_a_padic_number to u/saraedum/polynomial_representation_of_a_padic_number
comment:58 Changed 3 years ago by
 Commit changed from d7d5fb612c45ed3e51724f3834d194415cc0a122 to e51c0f50660a4fdb06071dba0551f5252984b171
 Work issues changed from merge conflicts, plugin error to plugin error
comment:59 Changed 3 years ago by
 Commit changed from e51c0f50660a4fdb06071dba0551f5252984b171 to b6457b1d48311d45bee32228bf989391b9754d88
Branch pushed to git repo; I updated commit sha1. New commits:
b6457b1  Moving SEEALSO to the end of the docstring

comment:60 Changed 3 years ago by
 Commit changed from b6457b1d48311d45bee32228bf989391b9754d88 to b81b7223f120e4714163c79cab851f62eccf4e19
Branch pushed to git repo; I updated commit sha1. New commits:
b81b722  Remove use of depraceted list()

comment:61 Changed 3 years ago by
 Branch changed from u/saraedum/polynomial_representation_of_a_padic_number to u/roed/polynomial_representation_of_a_padic_number
comment:62 Changed 3 years ago by
 Commit changed from b81b7223f120e4714163c79cab851f62eccf4e19 to 04a1579131d64cdc63ecf1babcba2350dec6497c
 Reviewers changed from Julian Rüth to Julian Rüth, David Roe
 Status changed from needs_review to positive_review
I'm happy with Julian's changes, and the patchbot is green. Positive review!
New commits:
04a1579  Fix NOTES blocks

comment:63 Changed 3 years ago by
 Branch changed from u/roed/polynomial_representation_of_a_padic_number to 04a1579131d64cdc63ecf1babcba2350dec6497c
 Resolution set to fixed
 Status changed from positive_review to closed
comment:64 Changed 2 years ago by
 Commit 04a1579131d64cdc63ecf1babcba2350dec6497c deleted
 Description modified (diff)
comment:65 Changed 2 years ago by
 Description modified (diff)
Patch does not merge. Please rebase.