Opened 11 years ago
Closed 10 years ago
#7879 closed enhancement (fixed)
Remove unnecessary signal handling for low prec mpfr operations.
Reported by: | robertwb | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | basic arithmetic | Keywords: | sd32 |
Cc: | Merged in: | sage-4.7.2.alpha3 | |
Authors: | Robert Bradshaw | Reviewers: | Alex Ghitza, Mariah Lenox, William Stein |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
_sig_on
and _sig_off
are more expensive than computing the result itself.
Apply only trac_7879-RR-signal.2.rebased_to_4.7.2.alpha2.patch to the Sage library.
Attachments (4)
Change History (22)
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Description modified (diff)
Changed 11 years ago by
comment:3 Changed 11 years ago by
- Reviewers set to Alex Ghitza
- Status changed from needs_review to needs_info
This looks good. I have one question: you are hardcoding threshold precisions after which the signals should be used, mostly 1000, but also 10000 in a couple of places. Would it be better to have a global variable PREC_THRESHOLD set to 1000 at the top of the file, and then use it or 10*PREC_THRESHOLD where needed? It would make it easier to change this in the future, since a search for it would be all that's needed.
I'm marking this as needs_info. If you think it's not worth the trouble, I'll put a positive review.
comment:4 Changed 10 years ago by
- Milestone changed from sage-4.6.2 to sage-duplicate/invalid/wontfix
- Status changed from needs_info to needs_review
I am proposing to close this ticket as invalid since #9678 makes sig_on()
and sig_off()
a lot faster. In any case, if you want to keep the patch, it needs rebasing (_sig_on
and _sig_off
must be replaced by sig_on()
and sig_off()
but there are more patch conflicts).
comment:5 Changed 10 years ago by
- Status changed from needs_review to positive_review
Since this ticket is marked sage-duplicate/invalid/wontfix plus has not been active for 4 months, I believe this ticket can be closed.
comment:6 Changed 10 years ago by
Robert, what is your opinion? sig_on()
and sig_off()
became a lot faster since #9678, but they still take up some cycles.
comment:7 follow-up: ↓ 9 Changed 10 years ago by
- Milestone changed from sage-duplicate/invalid/wontfix to sage-4.7.1
Thanks for taking a look at this. I've been waiting for 4.7 to be released to re-base this, and think it'd still worth having. Perhaps I'll put PREC_THRESHOLD in there while I'm at it, and maybe do some benchmarking.
comment:8 Changed 10 years ago by
- Status changed from positive_review to needs_work
comment:9 in reply to: ↑ 7 Changed 10 years ago by
Replying to robertwb:
and maybe do some benchmarking.
If you find useful ways to do benchmarks, please let me know. I might re-use those benchmarks to optimize sig_on()
and sig_off()
.
comment:10 Changed 10 years ago by
The patch adds one unrelated doctest. This doctest has been moved to #11344.
comment:11 Changed 10 years ago by
- Status changed from needs_work to needs_review
---------------------------------------------------------------------- | Sage Version 4.7, Release Date: 2011-05-23 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading Sage library. Current Mercurial branch is: bitrot sage: L = [RR(k) for k in range(1000)] sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 569 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 509 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 513 µs per loop sage: timeit("[a.sqrt() for a in L]", number=10000) 10000 loops, best of 3: 519 µs per loop sage: Exiting Sage (CPU time 0m19.94s, Wall time 1m9.75s). robertwb@sage:/scratch/robertwb/sage-4.7/devel/sage-bitrot$ hg qpush applying 7879-RR-signal.patch now at: 7879-RR-signal.patch robertwb@sage:/scratch/robertwb/sage-4.7/devel/sage-bitrot$ ../../sage -br [...] ---------------------------------------------------------------------- | Sage Version 4.7, Release Date: 2011-05-23 | | Type notebook() for the GUI, and license() for information. | ---------------------------------------------------------------------- Loading Sage library. Current Mercurial branch is: bitrot sage: L = [RR(k) for k in range(1000)] sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 498 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 498 µs per loop sage: %timeit [a.sqrt() for a in L] 625 loops, best of 3: 496 µs per loop sage: timeit("[a.sqrt() for a in L]", number=10000) 10000 loops, best of 3: 499 µs per loop
Looks like about a 5% win (much smaller than I remember it being before).
comment:12 follow-up: ↓ 13 Changed 10 years ago by
- Status changed from needs_review to needs_work
patch 7879-RR-signal.2.patch applied to sage-4.7.1.alpha1. Then did 'sage -b' followed by 'make testlong'. The following test failed:
eno% ./sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" ********************************************************************** File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/devel/sage/sage/rings/residue_field.pyx", line 670: sage: I = QQ[3^(1/3)].factor(5)[1][0]; I Exception raised: Traceback (most recent call last): File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_10[2]>", line 1, in <module> I = QQ[Integer(3)**(Integer(1)/Integer(3))].factor(Integer(5))[Integer(1)][Integer(0)]; I###line 670: sage: I = QQ[3^(1/3)].factor(5)[1][0]; I File "ring.pyx", line 167, in sage.rings.ring.Ring.__getitem__ (sage/rings/ring.c:2007) File "expression.pyx", line 8287, in sage.symbolic.expression.Expression.minpoly (sage/symbolic/expression.cpp:31950) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/lib/python/site-packages/sage/calculus/calculus.py", line 925, in minpoly if g(ex).simplify_trig().simplify_radical() == 0: File "expression.pyx", line 6546, in sage.symbolic.expression.Expression.simplify_trig (sage/symbolic/expression.cpp:24657) File "expression.pyx", line 460, in sage.symbolic.expression.Expression._maxima_ (sage/symbolic/expression.cpp:3674) File "sage_object.pyx", line 429, in sage.structure.sage_object.SageObject._interface_ (sage/structure/sage_object.c:3451) File "lazy_import.pyx", line 167, in sage.misc.lazy_import.LazyImport.__getattr__ (sage/misc/lazy_import.c:1353) File "lazy_import.pyx", line 110, in sage.misc.lazy_import.LazyImport._get_object (sage/misc/lazy_import.c:1023) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/lib/python/site-packages/sage/interfaces/maxima_lib.py", line 152, in <module> ecl_eval("#$%s$"%l) File "ecl.pyx", line 1224, in sage.libs.ecl.ecl_eval (sage/libs/ecl.c:6301) File "ecl.pyx", line 1239, in sage.libs.ecl.ecl_eval (sage/libs/ecl.c:6252) File "ecl.pyx", line 252, in sage.libs.ecl.ecl_safe_eval (sage/libs/ecl.c:2544) RuntimeError: ECL says: THROW: The catch MACSYMA-QUIT is undefined. ********************************************************************** File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/devel/sage/sage/rings/residue_field.pyx", line 672: sage: k = I.residue_field(); k Exception raised: Traceback (most recent call last): File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_10[3]>", line 1, in <module> k = I.residue_field(); k###line 672: sage: k = I.residue_field(); k File "element.pyx", line 328, in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2792) File "parent.pyx", line 277, in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2940) File "parent.pyx", line 175, in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2709) AttributeError: 'sage.symbolic.expression.Expression' object has no attribute 'residue_field' **********************************************************************
etc.
comment:13 in reply to: ↑ 12 Changed 10 years ago by
- Reviewers changed from Alex Ghitza to Alex Ghitza, William Stein
Replying to mariah:
patch 7879-RR-signal.2.patch applied to sage-4.7.1.alpha1. Then did 'sage -b' followed by 'make testlong'. The following test failed:
eno% ./sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" sage -t -long -force_lib "devel/sage/sage/rings/residue_field.pyx" ********************************************************************** File "/home/mariah/sage/sage-4.7.1.alpha1-x86_64-Linux-core2-fc-review-7879/devel/sage/sage/rings/residue_field.pyx", line 670: sage: I = QQ[3^(1/3)].factor(5)[1][0]; I Exception raised:
These errors don't occur for me with sage-4.7.2.alpha2.
comment:14 Changed 10 years ago by
- Status changed from needs_work to positive_review
I did a full test of Sage on another machine too with 4.7.2.alpha2, and everything works. Code looks good and addresses other people's issues...
comment:15 Changed 10 years ago by
- Keywords sd32 added
comment:16 Changed 10 years ago by
- Description modified (diff)
- Reviewers changed from Alex Ghitza, William Stein to Alex Ghitza, Mariah Lenox, William Stein
Changed 10 years ago by
Sage library patch. Robert's patch rebased to Sage 4.7.2.alpha2, because of fuzz 2.
comment:17 Changed 10 years ago by
- Description modified (diff)
Attached a rebased patch because of fuzz.
comment:18 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Signal handling is omitted for functions/precisions that were clearly and uniformly in the millisecond range or less.