#20777 closed enhancement (fixed)

faster __invert__ for integers

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.3
Component: basic arithmetic Keywords:
Cc: jdemeyer, nbruin Merged in:
Authors: Vincent Delecroix Reviewers: Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: 8b941ca (Commits) Commit: 8b941ca110b38a345e307ded0502d6be7f1ffd09
Dependencies: Stopgaps:

Description

Before

sage: a = 3
sage: %timeit ~a
1000000 loops, best of 3: 304 ns per loop

After

sage: a = 3
sage: %timeit ~a
1000000 loops, best of 3: 190 ns per loop

See also #20731

Change History (12)

comment:1 Changed 14 months ago by vdelecroix

  • Branch set to u/vdelecroix/20777
  • Commit set to f4d46bc077722c538c34c14f60f8e55eb029e713
  • Status changed from new to needs_review

New commits:

f4d46bcTrac 20777: Integer.__invert__

comment:2 Changed 14 months ago by jdemeyer

Why not keep the ZeroDivisionError?

comment:3 Changed 14 months ago by vdelecroix

For 5.inverse_of_unit()?

Last edited 14 months ago by vdelecroix (previous) (diff)

comment:4 Changed 14 months ago by vdelecroix

We might be more precise and raise either ZeroDivisionError for 0.inverse_of_unit() and ArithmeticError for ~n for |n| > 1... but it looks too complicated for me.

Last edited 14 months ago by vdelecroix (previous) (diff)

comment:5 Changed 13 months ago by jmantysalo

  • Status changed from needs_review to needs_work

I can confirm the speedup, I tested with

sum([~x for x in IntegerRange(1000,2000)])

However, I can also confirm the same doctest failure on src/sage/rings/polynomial/laurent_polynomial.pyx that patchbot says.

comment:6 Changed 13 months ago by git

  • Commit changed from f4d46bc077722c538c34c14f60f8e55eb029e713 to 8b941ca110b38a345e307ded0502d6be7f1ffd09

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

2b3941fTrac 20777: Integer.__invert__
8b941caTrac 20777: fix laurent polynomial inverse

comment:7 Changed 13 months ago by vdelecroix

Rebased and fixed. BTW the __invert__ of Laurent polynomial is completely wrong (see #20963).

comment:8 Changed 13 months ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:9 Changed 13 months ago by jmantysalo

  • Status changed from needs_review to positive_review

I can still confirm the speedup, and the doctest error is now gone.

I can not say that I really understand this part of Sage, but the code seems clear and simple enought to give positive review.

comment:10 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:11 Changed 13 months ago by jmantysalo

  • Reviewers set to Jori Mäntysalo
  • Status changed from needs_work to positive_review

comment:12 Changed 13 months ago by vbraun

  • Branch changed from u/vdelecroix/20777 to 8b941ca110b38a345e307ded0502d6be7f1ffd09
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.