Opened 13 years ago
Closed 13 years ago
#5795 closed enhancement (fixed)
[with patch, positive review] Improved performance of MPolynomialRing_libsingular.__call__()
Reported by: | SimonKing | Owned by: | SimonKing |
---|---|---|---|
Priority: | minor | Milestone: | sage-3.4.2 |
Component: | commutative algebra | Keywords: | MPolynomialRing_libsingular, coercion, call |
Cc: | malb | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
One comment in the __call__()
method of MPolynomialRing_libsingular states:
#TODO: We can do this faster without the dict
In fact, we can!
Without the patch, we have the following timings on sage.math:
sage: R=PolynomialRing(QQ,5,'x') sage: S=PolynomialRing(QQ,6,'x') sage: T=PolynomialRing(QQ,5,'y') sage: U=PolynomialRing(GF(2),5,'x') sage: p=R('x0*x1+2*x4+x3*x1^2')^4 sage: timeit('q=S(p)') 625 loops, best of 3: 341 µs per loop sage: timeit('q=T(p)') 625 loops, best of 3: 370 µs per loop sage: timeit('q=U(p)') 625 loops, best of 3: 451 µs per loop
With the patch, we have
sage: timeit('q=S(p)') 625 loops, best of 3: 328 µs per loop sage: timeit('q=T(p)') 625 loops, best of 3: 292 µs per loop sage: timeit('q=U(p)') 625 loops, best of 3: 396 µs per loop
So, the improvement is small, but it is an improvement.
Background:
In the original version, if the input of
__call__
is MPolynomial_libsingular, then thedict()
method of this polynomial was called in order to get the coefficients and exponent vectors.
In the new version, the underlying libsingular methods are called directly, which is faster. The price to pay is that currRing changes more often. I hope change of currRing is not too expensive (in my examples above, apparently it isn't).
Attachments (1)
Change History (7)
Changed 13 years ago by
comment:1 Changed 13 years ago by
- Summary changed from Improved performance of MPolynomialRing_libsingular.__call__() to [with patch, needs review] Improved performance of MPolynomialRing_libsingular.__call__()
comment:2 follow-up: ↓ 3 Changed 13 years ago by
Hi Simon, are you sure that you need all the rChangeCurrRing() business? All functions you are calling *should* be safe without it. This might save a few cycles.
Btw. you improved "conversion" not "coercion". IIRC this is how we call stuff happening in __call__
.
Other than that the patch looks goog, which version shall I apply the patch agains?
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 13 years ago by
Replying to malb:
Hi Simon, are you sure that you need all the rChangeCurrRing() business? All functions you are calling *should* be safe without it. This might save a few cycles.
First, I tried it without. But then there was a problem when converting a polynomial over QQ into a polynomial over GF(2) (or the other way around, I don't remember). I found that the line
c = co.si2sa(p_GetCoeff(_element, El_ring), El_ring, El_base)
did not work properly (always returning zero when it should return 1).
Then, in 'coefficients()', I found that 'rChangeCurrRing()' is used. I thought that it might be for a reason, and indeed the conversion works if 'El_ring' is 'currRing'.
But I do agree that it would be nice if it were possible to avoid the ring change.
Btw. you improved "conversion" not "coercion". IIRC this is how we call stuff happening in
__call__
.
Ok, I changed the key word...
Other than that the patch looks goog, which version shall I apply the patch agains?
3.4.1.rc3
Cheers,
Simon
comment:4 in reply to: ↑ 3 Changed 13 years ago by
comment:5 Changed 13 years ago by
- Summary changed from [with patch, needs review] Improved performance of MPolynomialRing_libsingular.__call__() to [with patch, positive review] Improved performance of MPolynomialRing_libsingular.__call__()
Doctests pass, good to go.
comment:6 Changed 13 years ago by
- Milestone changed from sage-4.0 to sage-3.4.2
- Resolution set to fixed
- Status changed from new to closed
Merged in Sage 3.4.2.alpha0.
Cheers,
Michael
Improved performance for polynomial conversion