Opened 7 years ago

Closed 7 years ago

#1558 closed defect (fixed)

[with patch, with positive review] more NTL wrapping, coefficient access and factoring

Reported by: jbmohler Owned by: somebody
Priority: major Milestone: sage-2.9.1
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The attached patch provides some more lines in ntl/decl.pxi to give direct access to some more of NTL. The are two main functionality improvements:

1) __getitem__ now has one less copy of the coefficient. This results in a noticable speed improvement for coefficients (and __hash__)

2) New _factor_ntl and _factor_pari in sage/rings/polynomial/polynomial_integer_dense_ntl.pyx enable us to have more fun with head-to-head comparisons of their factoring routines. It really seems a mixed bag about who is faster. I have a basic check in place that keeps the factoring all in NTL for small coefficients, but this really isn't the end of the story as far as the benchmarking goes.

# original
sage: R.<x>=ZZ[]
sage: f=x^2-1
sage: timeit f.factor()
1000 loops, best of 3: 784 µs per loop
# patched
sage: R.<x>=ZZ[]
sage: f=x^2-1
sage: timeit f.factor()
10000 loops, best of 3: 153 µs per loop

Attachments (1)

uni-factoring-ntl.patch (4.9 KB) - added by jbmohler 7 years ago.
a patch with better decision process for pari or ntl

Download all attachments as: .zip

Change History (4)

Changed 7 years ago by jbmohler

a patch with better decision process for pari or ntl

comment:1 Changed 7 years ago by rlm

  • Summary changed from [with patch] more NTL wrapping, coefficient access and factoring to [with patch, needs review] more NTL wrapping, coefficient access and factoring

comment:2 Changed 7 years ago by mhansen

  • Summary changed from [with patch, needs review] more NTL wrapping, coefficient access and factoring to [with patch, with positive review] more NTL wrapping, coefficient access and factoring

The patch applies, builds, and passes tests for me.

comment:3 Changed 7 years ago by rlm

  • Resolution set to fixed
  • Status changed from new to closed

merged in 2.9.1 rc2

Note: See TracTickets for help on using tickets.