Opened 13 years ago

Closed 13 years ago

#7868 closed defect (fixed)

Factoring in fraction fields

Reported by: spancratz Owned by: spancratz
Priority: minor Milestone: sage-4.3.1
Component: algebra Keywords: fraction field, factorization
Cc: John Cremona Merged in: sage-4.3.1.rc2
Authors: Sebastian Pancratz Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The following is a quote from John Cremona,

http://groups.google.com/group/sage-devel/browse_thread/thread/3638a91c0438f439

I define a rational function in two variables over a finite field:

sage: R.<x,y> = GF(2)[]
sage: f = x*y/(x+y)
sage: f.parent()
Fraction Field of Multivariate Polynomial Ring in x, y over Finite
Field of size 2

I try to factor it, and get this error:

sage: f.factor()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)

/home/masgaj/.sage/temp/host_56_150/17587/_home_masgaj__sage_init_sage_0.py
in <module>()

/local/jec/sage-4.3.rc0/local/lib/python2.6/site-packages/sage/rings/fraction_field_element.so
in sage.rings.fraction_field_element.FractionFieldElement.factor
(sage/rings/fraction_field_element.c:2972)()

/local/jec/sage-4.3.rc0/local/lib/python2.6/site-packages/sage/rings/polynomial/multi_polynomial_libsingular.so
in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.factor
(sage/rings/polynomial/multi_polynomial_libsingular.cpp:22701)()

NotImplementedError: proof = True factorization not implemented.  Call
factor with proof=False.

So I do what I am told, but:

sage: f.factor(proof=False)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/masgaj/.sage/temp/host_56_150/17587/_home_masgaj__sage_init_sage_0.py
in <module>()

TypeError: factor() takes no keyword arguments

Attachments (4)

trac7868_20100107.patch (1.3 KB) - added by spancratz 13 years ago.
7868-fix.patch (1.7 KB) - added by Robert Bradshaw 13 years ago.
replaces previous
7868-fix.2.patch (1.7 KB) - added by John Cremona 13 years ago.
replaces previous
trac7868_rb.patch (1.8 KB) - added by spancratz 13 years ago.
Rebase to 4.3.1.rc0

Download all attachments as: .zip

Change History (19)

Changed 13 years ago by spancratz

Attachment: trac7868_20100107.patch added

comment:1 Changed 13 years ago by spancratz

Status: newneeds_review

The patch I attached earlier changes the behaviour of the factoring method in fraction fields, passing on additional parameters to the factoring method of the underlying ring rather than ignoring them.

I've now tested the patch (without the long option), and this did not return any test failures.

Sebastian

comment:2 Changed 13 years ago by spancratz

Owner: changed from Alex Ghitza to spancratz

comment:3 Changed 13 years ago by Robert Bradshaw

Status: needs_reviewpositive_review

Looks good to me.

comment:4 Changed 13 years ago by Robert Bradshaw

Status: positive_reviewneeds_work

Well, the code looks good, but we need a test showing the behavior is fixed.

comment:5 Changed 13 years ago by John Cremona

I agree with Robert's point, which I was going to make too. But for me the code does not apply to either 4.3 or 4.3.1.apha1. Sebastian, did you make the patch on top of your other changes to the fraction field code? Robert, did you apply the patch?

comment:6 Changed 13 years ago by Robert Bradshaw

Status: needs_workneeds_review

No, I didn't actually apply it... (I've got a lot of fraction field changes that I needed to pop off first, and no, it's not applying for me either. I don't see why.) I've posted an updated one that should merge onto 4.3 fine, with the new doctest.

comment:7 Changed 13 years ago by John Cremona

Status: needs_reviewneeds_work

OOps, that second patch is empty!

Changed 13 years ago by Robert Bradshaw

Attachment: 7868-fix.patch added

replaces previous

comment:8 Changed 13 years ago by Robert Bradshaw

Oops...

comment:9 Changed 13 years ago by Robert Bradshaw

Status: needs_workneeds_review

Changed 13 years ago by John Cremona

Attachment: 7868-fix.2.patch added

replaces previous

comment:10 Changed 13 years ago by John Cremona

Status: needs_reviewpositive_review

I deleted the lines sage: f.parent() in the patch since you omitted its output. If this works for you: positive review. Thanks!

comment:11 in reply to:  10 Changed 13 years ago by spancratz

Great. This all happened rather swiftly!

Sebastian

Replying to cremona:

I deleted the lines sage: f.parent() in the patch since you omitted its output. If this works for you: positive review. Thanks!

comment:12 Changed 13 years ago by Robert Miller

Status: positive_reviewneeds_work

Sorry, there's a conflict:

patching file sage/rings/fraction_field_element.pyx
Hunk #1 FAILED at 212
1 out of 2 hunks FAILED -- saving rejects to file sage/rings/fraction_field_element.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 7868-fix.2.patch

Changed 13 years ago by spancratz

Attachment: trac7868_rb.patch added

Rebase to 4.3.1.rc0

comment:13 Changed 13 years ago by Robert Miller

Authors: spancratzSebastian Pancratz
Reviewers: John Cremona
Status: needs_workneeds_review

comment:14 Changed 13 years ago by Robert Miller

Status: needs_reviewpositive_review

comment:15 Changed 13 years ago by Robert Miller

Merged in: sage-4.3.1.rc2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.