Opened 5 years ago

Closed 3 years ago

# bug in LaurentPolynomial.factor

Reported by: Owned by: mmarco major sage-duplicate/invalid/wontfix commutative algebra laurent polynomials rws, vdelecroix, stumpc5, etn40ff Vincent Delecroix N/A

fixed in #20214

Factoring multibvariable laurent polynomials over the integers might fail as follows:

sage: L.<x,y> = LaurentPolynomialRing(ZZ)
sage: f = y + x/y
sage: f.factor()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-7-429743410e57> in <module>()
----> 1 f.factor()

/home/mmarco/sage/src/sage/rings/polynomial/laurent_polynomial.pyx in sage.rings.polynomial.laurent_polynomial.LaurentPolynomial_mpair.factor (/home/mmarco/sage/src/build/cythonized/sage/rings/polynomial/laurent_polynomial.c:29472)()
2511         """
2512         pf = self._poly.factor()
-> 2513         u = self.parent(pf.unit().dict()) # self.parent won't currently take polynomials
2514
2515         g = self.parent().gens()

/home/mmarco/sage/src/sage/structure/element.pyx in sage.structure.element.Element.__getattr__ (/home/mmarco/sage/src/build/cythonized/sage/structure/element.c:4649)()
411             dummy_error_message.name = name
412             raise dummy_attribute_error
--> 413         return getattr_from_other_class(self, P._abstract_element_class, name)
414
415     def __dir__(self):

/home/mmarco/sage/src/sage/structure/misc.pyx in sage.structure.misc.getattr_from_other_class (/home/mmarco/sage/src/build/cythonized/sage/structure/misc.c:1870)()
257         dummy_error_message.cls = type(self)
258         dummy_error_message.name = name
--> 259         raise dummy_attribute_error
260     if isinstance(attribute, methodwrapper):
261         dummy_error_message.cls = type(self)

AttributeError: 'sage.rings.integer.Integer' object has no attribute 'dict'

### comment:1 Changed 5 years ago by mmarco

• Authors set to Miguel Marco
• Cc rws vdelecroix stumpc5 etn40ff added
• Component changed from PLEASE CHANGE to commutative algebra
• Description modified (diff)
• Status changed from new to needs_review
• Type changed from PLEASE CHANGE to defect

### comment:2 Changed 5 years ago by mmarco

• Branch set to u/mmarco/bug_in_laurentpolynomial_factor

### comment:3 Changed 5 years ago by vdelecroix

Related to #20214. Wouldn't be better that the unit of the factorization actually belong to the ring?

New commits:

 ​6534464 Treat case where base ring is not a field

### comment:4 Changed 5 years ago by mmarco

The unit must belong to the LaurentPolynomialRing?, since there are units that don't belong to the base ring.

### comment:5 Changed 5 years ago by vdelecroix

I am not sure if must is appropriate. What about actually fixing this factorization inconsistency instead of introducing these if/else?

### comment:6 Changed 5 years ago by mmarco

In the case of Laurent Polynonials, the variables are units. So we cannot enforce the unit to live in the base ring. Consider, for instance, the element 2*L^2 + 2*L in the LaurentPolynomialRing? of 1 variable over the integers. Its factorization is L * 2 * (L+1), where:

• L is the unit. It doesn't live in the base ring (which is ZZ)
• 2 is a factor that happens to live in the base ring, but is not a unit.
• L + 1 is a factor that doesn't live in the base ring and is not a unit.

As you say, all these if/else wouldn't be necessary if the factorization of polynomials would return the unit as an element of the polynomial ring (and hence, they would have a .dict() method that would prevent this bug. My proposal for #20214 would automatically make this patch unnecessary, but it seems that my opinion is not shared by many people there.

### comment:7 Changed 5 years ago by vdelecroix

Hi Miguel,

I made a branch at #20214 that correct automatically the bug you had. Please have a look...

Vincent

### comment:8 follow-up: ↓ 9 Changed 4 years ago by tscrim

• Milestone changed from sage-7.3 to sage-8.0
• Status changed from needs_review to needs_info

Is this subsumed by #20214?

### comment:9 in reply to: ↑ 8 Changed 3 years ago by vdelecroix

Is this subsumed by #20214?

yes!

### comment:10 Changed 3 years ago by tscrim

• Milestone changed from sage-8.0 to sage-duplicate/invalid/wontfix
• Status changed from needs_info to needs_review

Then I propose closing this.

### comment:11 Changed 3 years ago by vdelecroix

• Authors Miguel Marco deleted
• Branch u/mmarco/bug_in_laurentpolynomial_factor deleted
• Description modified (diff)
• Reviewers set to Vincent Delecroix
• Status changed from needs_review to positive_review

### comment:12 Changed 3 years ago by embray

• Resolution set to invalid
• Status changed from positive_review to closed

Presuming these are all correctly reviewed as either duplicate, invalid, or wontfix.

### comment:13 Changed 20 months ago by vdelecroix

Since #20214 is kept open, I took the liberty to reopen this one as #29173.

Note: See TracTickets for help on using tickets.