Ticket #2179 (new defect)

Opened 10 months ago

Last modified 5 months ago

[with patch, needs work] implementation mpoly factoring with coefficients in ZZ

Reported by: jbmohler Assigned to: jbmohler
Priority: major Milestone: sage-3.2.2
Component: commutative algebra Keywords: editor_wstein
Cc: ncalexan

Description

Here's a pure python implementation of an algorithm to factor polynomials over ZZ using kronecker's trick (specializing a variable to a large prime reduces you to a poly of fewer variables). Note that this also fills in an implementation of factoring over ZZ[x,y] --- we don't have any implementation at all for this currently. It's also faster than singular (over QQ) for some cases.

Here's an example with my favorite trouble-maker for singular.

this patch:

sage: R.<p10,g0,g1,g2,g3,g4,X1,X2>=ZZ[]
sage: t=-p10^170*X1^10*X2^10+p10^130*X1^10*X2^5+p10^130*X1^5*X2^10-p10^90*X1^5*X2^5+p10^80*X1^5*X2^5-p10^40*X1^5-p10^40*X2^5+1
sage: time t.factor()
CPU times: user 0.11 s, sys: 0.00 s, total: 0.12 s
Wall time: 0.12
(-1) * (p10^8*X2 - 1) * (p10^8*X1 - 1) * (p10^18*X1*X2 - 1) * (p10^32*X2^4 + p10^24*X2^3 + p10^16*X2^2 + p10^8*X2 + 1) * (p10^32*X1^4 + p10^24*X1^3 + p10^16*X1^2 + p10^8*X1 + 1) * (p10^72*X1^4*X2^4 + p10^54*X1^3*X2^3 + p10^36*X1^2*X2^2 + p10^18*X1*X2 + 1)

singular:

sage: R.<p10,g0,g1,g2,g3,g4,X1,X2>=QQ[]
sage: t=-p10^170*X1^10*X2^10+p10^130*X1^10*X2^5+p10^130*X1^5*X2^10-p10^90*X1^5*X2^5+p10^80*X1^5*X2^5-p10^40*X1^5-p10^40*X2^5+1
sage: time t.factor()
CPU times: <longer than I wanted to wait>

Attachments

mpoly-factor.patch (11.5 kB) - added by jbmohler on 02/16/2008 12:46:15 PM.

Change History

02/16/2008 12:46:15 PM changed by jbmohler

  • attachment mpoly-factor.patch added.

02/16/2008 12:46:50 PM changed by jbmohler

  • milestone set to sage-2.10.2.

02/16/2008 12:57:13 PM changed by jbmohler

  • owner changed from was to malb.
  • component changed from algebraic geometry to commutative algebra.

02/18/2008 11:44:23 AM changed by ncalexan

  • cc set to ncalexan.
  • summary changed from [with patch] implementation mpoly factoring with coefficients in ZZ to [with patch, with cautiously optimistic review] implementation mpoly factoring with coefficients in ZZ.

I am no expert on factoring algorithms, but this does seem to fill a needed hole, is reasonably implemented, and has some rudimentary doctests.

I say apply.

Isn't this whole area going to be addressed soon anyway, so that this code will likely be reviewed and enhanced shortly?

02/19/2008 06:51:53 AM changed by was

  • summary changed from [with patch, with cautiously optimistic review] implementation mpoly factoring with coefficients in ZZ to [with patch, negative review] implementation mpoly factoring with coefficients in ZZ.

REVIEW:

This absolutely should not be applied for the simple reason that the file sage/rings/polynomial/multi_polynomial_factor.py introduces 7 new functions that have 0 docstrings and 0 doctests, and essentially 0 documentation. My understanding, which I want to push very hard, is that no new code should go into Sage that reduces the overall doctest coverage score.

Joel -- can you add doctests and docstrings for every single function multi_polynomial_factor.py? I do that with *all* new code I write for Sage.

02/20/2008 08:49:13 AM changed by was

  • summary changed from [with patch, negative review] implementation mpoly factoring with coefficients in ZZ to [with patch, accepted pending addition of doctests] implementation mpoly factoring with coefficients in ZZ.

02/25/2008 09:26:19 AM changed by jbmohler

I'm reviewing my own code today. Expect a new patch to be submitted as of later today. I just thought I'd mention that here in case someone was looking at this code today.

02/26/2008 06:18:12 PM changed by jbmohler

  • milestone changed from sage-2.10.3 to sage-3.0.

I'm retracting this for the moment. As expected, once I started looking at this code I found so many ways to improve it and totally got sucked in again and want to make it better.

03/28/2008 04:44:52 AM changed by malb

  • owner changed from malb to jbmohler.

03/31/2008 10:12:53 PM changed by was

06/03/2008 07:22:18 AM changed by malb

  • summary changed from [with patch, accepted pending addition of doctests] implementation mpoly factoring with coefficients in ZZ to [with patch, needs work] implementation mpoly factoring with coefficients in ZZ.

06/19/2008 09:41:57 PM changed by craigcitro

  • keywords set to editor_wstein.
  • summary changed from [with patch, needs work] implementation mpoly factoring with coefficients in ZZ to [with patch, under review by burcin before 6/27] implementation mpoly factoring with coefficients in ZZ.

07/04/2008 10:32:55 AM changed by burcin

  • summary changed from [with patch, under review by burcin before 6/27] implementation mpoly factoring with coefficients in ZZ to [with patch, needs work] implementation mpoly factoring with coefficients in ZZ.

Joel says he has an improved patch with various additional tricks to filter easy factors. I will wait for an updated patch to review this.