Opened 9 years ago

Closed 4 years ago

Last modified 3 years ago

#13591 closed enhancement (fixed)

Improve padic add_bigoh

Reported by: saraedum Owned by: saraedum
Priority: trivial Milestone: sage-7.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:

Status badges

Description (last modified by saraedum)

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)

trac_13591.patch (14.2 KB) - added by saraedum 9 years ago.

Download all attachments as: .zip

Change History (44)

Changed 9 years ago by saraedum

comment:1 Changed 9 years ago by saraedum

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by saraedum

  • Authors set to Julian Rueth

comment:3 follow-up: Changed 9 years ago by roed

  • 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(pn) (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.
Last edited 6 years ago by vdelecroix (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 9 years ago by saraedum

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 saraedum

  • Status changed from needs_info to needs_review

comment:6 Changed 9 years ago by saraedum

  • Status changed from needs_review to needs_work

comment:7 Changed 9 years ago by saraedum

  • Owner changed from roed to saraedum

comment:8 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:11 Changed 7 years ago by saraedum

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

  • Description modified (diff)
  • Keywords sd59 added
  • Status changed from needs_work to needs_review

comment:13 Changed 7 years ago by git

  • Commit set to 6758b1d7f60a2ffd28636234fcc5c55cb90e8a37

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

52e4024Trac 13591: improve add_bigoh for padics
60cb928Added sig_on/sig_off to prevent a crash in p-adic add_bigoh()
3b222dfRemoved duplicate copyright header
15a2722a padic element's add_bigoh() can now handle negative inputs (see #13591)
6758b1dAdded tests for padic add_bigoh()

comment:14 Changed 7 years ago by saraedum

  • Status changed from needs_review to needs_work

comment:15 Changed 7 years ago by git

  • Commit changed from 6758b1d7f60a2ffd28636234fcc5c55cb90e8a37 to 2c847138d49bbb1ff7acf4537a7c8be065d7db5f

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

fb83d40Fix handling of +infinity in add_bigoh() of padics.
2c84713fix _test_add_bigoh() for capped relative padics

comment:16 Changed 7 years ago by saraedum

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

Doesn't the sig_on()/sig_off() slow it down too much?

comment:18 Changed 7 years ago by saraedum

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

  • Work issues set to performance (sig_on/sig_off)

comment:20 Changed 7 years ago by saraedum

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

  • Commit changed from 2c847138d49bbb1ff7acf4537a7c8be065d7db5f to bfb13b1915c0bf67bf9b7a5b63db7cb5db2c9542

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

d5cdbf9Merge branch 'develop' into ticket/13591
cd0c153Removed unnecessary sig_on()/sig_off() from p-adics code
bfb13b1made PowComputer.pow_mpz_t_tmp more robust to invalid/extreme inputs

comment:22 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:23 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:24 Changed 7 years ago by git

  • Commit changed from bfb13b1915c0bf67bf9b7a5b63db7cb5db2c9542 to 15a0134b10ac7bbef4579c2774918f3d3d9ff886

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

15a0134Merge branch 'develop' into ticket/13591

comment:25 Changed 7 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:26 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Should be rebased on top of #17090.

comment:27 follow-up: Changed 6 years ago by git

  • Commit changed from 15a0134b10ac7bbef4579c2774918f3d3d9ff886 to 2d873530c02870132c1ff32ff502710311507ce3

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

2d87353Merge branch 'develop' into t/13591/ticket/13591

comment:28 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:29 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply, needs rebase

comment:30 Changed 6 years ago by git

  • Commit changed from 2d873530c02870132c1ff32ff502710311507ce3 to 5d7dbb90c60449ef57316c4d7cf4ea98ab184950

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

9cd2d5dFixup the preceding merge.
5d7dbb9Merge branch 'develop' into t/13591/ticket/13591

comment:31 in reply to: ↑ 27 Changed 6 years ago by saraedum

Replying to git:

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

2d87353Merge 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:

9cd2d5dFixup the preceding merge.
5d7dbb9Merge branch 'develop' into t/13591/ticket/13591

comment:32 Changed 6 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:33 Changed 5 years ago by aly.deines

  • Keywords sd71 added

Looks good!

comment:34 Changed 5 years ago by aly.deines

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

  • Milestone changed from sage-6.4 to sage-7.2

comment:36 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, probably #14304

comment:37 Changed 4 years ago by roed

  • Branch changed from u/saraedum/ticket/13591 to u/roed/ticket/13591

comment:38 Changed 4 years ago by git

  • Commit changed from 5d7dbb90c60449ef57316c4d7cf4ea98ab184950 to 782df2ea0c9b4f81be239847aacc971c2eaaa4ad

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

782df2eFix prec_cap -> ram_prec_cap

comment:39 Changed 4 years ago by git

  • Commit changed from 782df2ea0c9b4f81be239847aacc971c2eaaa4ad to 538b48c44da6090ad2899d31234b33d35ba846ad

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

538b48cFixing problems revealed by doctests

comment:40 Changed 4 years ago by roed

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

  • Authors changed from Julian Rueth to Julian Rueth, David Roe
  • 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 vbraun

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

  • Authors changed from Julian Rueth, David Roe to Julian Rüth, David Roe
  • Commit 538b48c44da6090ad2899d31234b33d35ba846ad deleted
Note: See TracTickets for help on using tickets.