Opened 10 years ago

Closed 10 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: cremona Merged in: sage-4.3.1.rc2
Authors: Sebastian Pancratz Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

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 10 years ago.
7868-fix.patch (1.7 KB) - added by robertwb 10 years ago.
replaces previous
7868-fix.2.patch (1.7 KB) - added by cremona 10 years ago.
replaces previous
trac7868_rb.patch (1.8 KB) - added by spancratz 10 years ago.
Rebase to 4.3.1.rc0

Download all attachments as: .zip

Change History (19)

Changed 10 years ago by spancratz

comment:1 Changed 10 years ago by spancratz

  • Status changed from new to needs_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 10 years ago by spancratz

  • Owner changed from AlexGhitza to spancratz

comment:3 Changed 10 years ago by robertwb

  • Status changed from needs_review to positive_review

Looks good to me.

comment:4 Changed 10 years ago by robertwb

  • Status changed from positive_review to needs_work

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

comment:5 Changed 10 years ago by 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 10 years ago by robertwb

  • Status changed from needs_work to needs_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 10 years ago by cremona

  • Status changed from needs_review to needs_work

OOps, that second patch is empty!

Changed 10 years ago by robertwb

replaces previous

comment:8 Changed 10 years ago by robertwb

Oops...

comment:9 Changed 10 years ago by robertwb

  • Status changed from needs_work to needs_review

Changed 10 years ago by cremona

replaces previous

comment:10 follow-up: Changed 10 years ago by cremona

  • Status changed from needs_review to positive_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 10 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 10 years ago by rlm

  • Status changed from positive_review to needs_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 10 years ago by spancratz

Rebase to 4.3.1.rc0

comment:13 Changed 10 years ago by rlm

  • Authors changed from spancratz to Sebastian Pancratz
  • Reviewers set to John Cremona
  • Status changed from needs_work to needs_review

comment:14 Changed 10 years ago by rlm

  • Status changed from needs_review to positive_review

comment:15 Changed 10 years ago by rlm

  • Merged in set to sage-4.3.1.rc2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.