#13591 closed enhancement (fixed)
Improve padic add_bigoh
Reported by:  saraedum  Owned by:  saraedum 

Priority:  trivial  Milestone:  sage7.2 
Component:  padics  Keywords:  sd59, days71 
Cc:  roed  Merged in:  
Authors:  Julian Rüth, David Roe  Reviewers:  Aly Deines, Julian Rüth 
Report Upstream:  N/A  Work issues:  
Branch:  538b48c (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
add_bigoh()
handled negative inputs very inconsistently. Now it tries to
bring the element into the fraction field and apply add_bigoh()
there.
Additionally, add_bigoh()
did not work for infinity which can be annoying sometimes:
sage: K.one().add_bigoh(infinity) TypeError: unable to coerce <class 'sage.rings.infinity.PlusInfinity'> to an integer
Attachments (1)
Change History (44)
Changed 9 years ago by
comment:1 Changed 9 years ago by
 Status changed from new to needs_review
comment:2 Changed 9 years ago by
comment:3 followup: ↓ 4 Changed 9 years ago by
 Status changed from needs_review to needs_info
 making it work for infinity is great
 I'm fairly ambivalent about what happens when you add_big_oh(1) to a ring element. Certainly the current behavior is inconsistent and should be changed. But I usually think about x.add_bigoh(n) as being equivalent to x + O(p^{n}) (just more efficient). If that's true then both of your K.one().add_bigoh(1) should return O(3^{1}) in the fraction field. Another benefit of this choice is that the behavior is the same for elements of K and K.fraction_field(), which I think is a good thing when we can manage it. Why do you think we should raise an error when there's a sensible answer?
 I do think that ZpFM(3,20).one().add_bigoh(10) should not raise an error. The whole point of fixed modulus elements is that they don't worry about precision.
comment:4 in reply to: ↑ 3 Changed 9 years ago by
Replying to roed:
 If that's true then both of your K.one().add_bigoh(1) should return
O(3^1)
in the fraction field.
Right. This makes more sense. The problem was that it returned O(3^1)
in the ring but not in the fraction field — probably we should not allow elements with negative absolute precision in the ring (but this will be a different ticket). I'll change it accordingly.
 I do think that ZpFM(3,20).one().add_bigoh(10) should not raise an error. The whole point of fixed modulus elements is that they don't worry about precision.
Ok. I'll change that. I have no strong opinion about it.
comment:5 Changed 9 years ago by
 Status changed from needs_info to needs_review
comment:6 Changed 9 years ago by
 Status changed from needs_review to needs_work
comment:7 Changed 9 years ago by
 Owner changed from roed to saraedum
comment:8 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:9 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:10 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:11 Changed 7 years ago by
 Branch set to u/saraedum/ticket/13591
 Created changed from 10/11/12 19:07:39 to 10/11/12 19:07:39
 Modified changed from 05/06/14 15:20:58 to 05/06/14 15:20:58
comment:12 Changed 7 years ago by
 Description modified (diff)
 Keywords sd59 added
 Status changed from needs_work to needs_review
comment:13 Changed 7 years ago by
 Commit set to 6758b1d7f60a2ffd28636234fcc5c55cb90e8a37
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
52e4024  Trac 13591: improve add_bigoh for padics

60cb928  Added sig_on/sig_off to prevent a crash in padic add_bigoh()

3b222df  Removed duplicate copyright header

15a2722  a padic element's add_bigoh() can now handle negative inputs (see #13591)

6758b1d  Added tests for padic add_bigoh()

comment:14 Changed 7 years ago by
 Status changed from needs_review to needs_work
comment:15 Changed 7 years ago by
 Commit changed from 6758b1d7f60a2ffd28636234fcc5c55cb90e8a37 to 2c847138d49bbb1ff7acf4537a7c8be065d7db5f
comment:16 Changed 7 years ago by
 Modified changed from 06/28/14 05:05:02 to 06/28/14 05:05:02
 Status changed from needs_work to needs_review
comment:17 Changed 7 years ago by
Doesn't the sig_on()/sig_off() slow it down too much?
comment:18 Changed 7 years ago by
 Status changed from needs_review to needs_info
Thanks for having a look. I'll perform some tests and see whether it makes a difference.
comment:19 Changed 7 years ago by
 Work issues set to performance (sig_on/sig_off)
comment:20 Changed 7 years ago by
 Cc roed added
 Status changed from needs_info to needs_review
 Work issues performance (sig_on/sig_off) deleted
I removed the sig_on()/sig_off()
in creduce. The one in prime_pow
did not make a difference in my tests so I kept it and added some comments.
David, could you review this? It touches things that you wrote and maybe I do not fully understand the implications of this last change.
comment:21 Changed 7 years ago by
 Commit changed from 2c847138d49bbb1ff7acf4537a7c8be065d7db5f to bfb13b1915c0bf67bf9b7a5b63db7cb5db2c9542
comment:22 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:23 Changed 7 years ago by
 Status changed from needs_review to needs_work
comment:24 Changed 7 years ago by
 Commit changed from bfb13b1915c0bf67bf9b7a5b63db7cb5db2c9542 to 15a0134b10ac7bbef4579c2774918f3d3d9ff886
Branch pushed to git repo; I updated commit sha1. New commits:
15a0134  Merge branch 'develop' into ticket/13591

comment:25 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:26 Changed 7 years ago by
 Status changed from needs_review to needs_work
Should be rebased on top of #17090.
comment:27 followup: ↓ 31 Changed 6 years ago by
 Commit changed from 15a0134b10ac7bbef4579c2774918f3d3d9ff886 to 2d873530c02870132c1ff32ff502710311507ce3
Branch pushed to git repo; I updated commit sha1. New commits:
2d87353  Merge branch 'develop' into t/13591/ticket/13591

comment:28 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:29 Changed 6 years ago by
 Status changed from needs_review to needs_work
does not apply, needs rebase
comment:30 Changed 6 years ago by
 Commit changed from 2d873530c02870132c1ff32ff502710311507ce3 to 5d7dbb90c60449ef57316c4d7cf4ea98ab184950
comment:31 in reply to: ↑ 27 Changed 6 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
2d87353 Merge branch 'develop' into t/13591/ticket/13591
I messed up this merge. I introduced a new commit to fix the merge. Sorry for that.
New commits:
9cd2d5d  Fixup the preceding merge.

5d7dbb9  Merge branch 'develop' into t/13591/ticket/13591

comment:32 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:34 Changed 5 years ago by
 Keywords days71 added; sd71 removed
 Reviewers set to Aly Deines
 Status changed from needs_review to positive_review
comment:35 Changed 5 years ago by
 Milestone changed from sage6.4 to sage7.2
comment:36 Changed 5 years ago by
 Status changed from positive_review to needs_work
Merge conflict, probably #14304
comment:37 Changed 4 years ago by
 Branch changed from u/saraedum/ticket/13591 to u/roed/ticket/13591
comment:38 Changed 4 years ago by
 Commit changed from 5d7dbb90c60449ef57316c4d7cf4ea98ab184950 to 782df2ea0c9b4f81be239847aacc971c2eaaa4ad
Branch pushed to git repo; I updated commit sha1. New commits:
782df2e  Fix prec_cap > ram_prec_cap

comment:39 Changed 4 years ago by
 Commit changed from 782df2ea0c9b4f81be239847aacc971c2eaaa4ad to 538b48c44da6090ad2899d31234b33d35ba846ad
Branch pushed to git repo; I updated commit sha1. New commits:
538b48c  Fixing problems revealed by doctests

comment:40 Changed 4 years ago by
 Status changed from needs_work to needs_review
Only the additional fixes on top of what Aly reviewed need to be looked at.
comment:41 Changed 4 years ago by
 Reviewers changed from Aly Deines to Aly Deines, Julian Rüth
 Status changed from needs_review to positive_review
comment:42 Changed 4 years ago by
 Branch changed from u/roed/ticket/13591 to 538b48c44da6090ad2899d31234b33d35ba846ad
 Resolution set to fixed
 Status changed from positive_review to closed
comment:43 Changed 3 years ago by
 Commit 538b48c44da6090ad2899d31234b33d35ba846ad deleted