Opened 10 years ago

Closed 10 years ago

#5928 closed defect (fixed)

[with patch, positive review] binary operations on factorisations should coerce factors into a common universe

Reported by: AlexGhitza Owned by: AlexGhitza
Priority: major Milestone: sage-3.4.2
Component: factorization Keywords: multiplication factorization coercion
Cc: cremona Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

This was uncovered at #5921. Observe:

sage: P.<x> = ZZ
sage: f = 2*x + 2
sage: c = f.content()
sage: g = f//c
sage: F1 = c.factor(); [type(a[0]) for a in F1]
[<type 'sage.rings.integer.Integer'>]
sage: F2 = g.factor(); [type(a[0]) for a in F2]
[<type 'sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint'>]
sage: F1*F2
2 * (x + 1)
sage: [type(a[0]) for a in F1*F2]
[<type 'sage.rings.integer.Integer'>,
 <type 'sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint'>]

I think that multiplying two factorisations should make sure that the factors can be coerced into a common universe, so that all factors have the same parent. If that's impossible, then an error should be thrown.

Attachments (1)

trac_5928.patch (6.6 KB) - added by AlexGhitza 10 years ago.

Download all attachments as: .zip

Change History (4)

comment:1 Changed 10 years ago by AlexGhitza

  • Status changed from new to assigned
  • Summary changed from multiplication of factorisations should coerce factors into a common universe to [with patch, needs review] binary operations on factorisations should coerce factors into a common universe

The attached patch does this for multiplication, gcd, and lcm. The other operations either inherit it or were dealing with this already.

Changed 10 years ago by AlexGhitza

comment:2 Changed 10 years ago by cremona

  • Summary changed from [with patch, needs review] binary operations on factorisations should coerce factors into a common universe to [with patch, positive review] binary operations on factorisations should coerce factors into a common universe

This patch looks good, applies to 3.4.2.alpha0 and tests in sage/structure pass as well as those in sage/rings/*.py (I did not go into subdirectories).

I was a little disappointed by this:

sage: R.<x> = ZZ[]
sage: S.<y> = QQ[]
sage: f = x^2-1
sage: g = y^3-1
sage: f.factor()
(x - 1) * (x + 1)
sage: g.factor()
(y - 1) * (y^2 + y + 1)
sage: f.factor() * g.factor()
(1) * (y - 1) * (x - 1) * (x + 1) * (y^2 + y + 1)
sage: (f.factor() * g.factor()).universe()
Category of objects

and in fact coercion is not clever enough to allow x*y here. but it does work if you do

sage: S.<x> = QQ[]
sage: y=S.gen(0)
sage: g = y^3-1
sage: f.factor() * g.factor()
(x + 1) * (x - 1)^2 * (x^2 + x + 1)

-- i.e. you have to define the two rings with the same name of the variable even if you use a different name for input. Weird, but it is not going to stop this patch!

comment:3 Changed 10 years ago by mabshoff

  • Milestone changed from sage-4.0 to sage-3.4.2
  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in Sage 3.4.2.rc0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.