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:

Status badges

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 the dict() 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)

MPolyRingCall.patch (3.3 KB) - added by SimonKing 13 years ago.
Improved performance for polynomial conversion

Download all attachments as: .zip

Change History (7)

Changed 13 years ago by SimonKing

Improved performance for polynomial conversion

comment:1 Changed 13 years ago by SimonKing

  • Summary changed from Improved performance of MPolynomialRing_libsingular.__call__() to [with patch, needs review] Improved performance of MPolynomialRing_libsingular.__call__()

comment:2 follow-up: Changed 13 years ago by 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.

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: Changed 13 years ago by SimonKing

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 SimonKing

Replying to SimonKing:

Replying to malb:

Other than that the patch looks goog, which version shall I apply the patch agains?

3.4.1.rc3

Ooops, it should be 3.4.1.rc2, I think.

comment:5 Changed 13 years ago by malb

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

  • 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

Note: See TracTickets for help on using tickets.