Opened 2 years ago
Closed 23 months ago
#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 2 years ago by
- Branch set to u/vdelecroix/20777
- Commit set to f4d46bc077722c538c34c14f60f8e55eb029e713
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
Why not keep the ZeroDivisionError
?
comment:3 Changed 2 years ago by
For 5.inverse_of_unit()
?
comment:4 Changed 2 years ago by
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.
comment:5 Changed 23 months ago by
- 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 23 months ago by
- Commit changed from f4d46bc077722c538c34c14f60f8e55eb029e713 to 8b941ca110b38a345e307ded0502d6be7f1ffd09
comment:7 Changed 23 months ago by
Rebased and fixed. BTW the __invert__
of Laurent polynomial is completely wrong (see #20963).
comment:8 Changed 23 months ago by
- Status changed from needs_work to needs_review
comment:9 Changed 23 months ago by
- 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:11 Changed 23 months ago by
- Reviewers set to Jori Mäntysalo
- Status changed from needs_work to positive_review
comment:12 Changed 23 months ago by
- Branch changed from u/vdelecroix/20777 to 8b941ca110b38a345e307ded0502d6be7f1ffd09
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac 20777: Integer.__invert__