Opened 3 years ago

Closed 8 months ago

#20607 closed defect (invalid)

bug in LaurentPolynomial.factor

Reported by: mmarco Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: commutative algebra Keywords: laurent polynomials
Cc: rws, vdelecroix, stumpc5, etn40ff Merged in:
Authors: Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

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'

Change History (12)

comment:1 Changed 3 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)
  • Keywords laurent polynomials added
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 3 years ago by mmarco

  • Branch set to u/mmarco/bug_in_laurentpolynomial_factor

comment:3 Changed 3 years ago by vdelecroix

  • Commit set to 653446429df8f6187b4de63ef6fad9642408bc93

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


New commits:

6534464Treat case where base ring is not a field

comment:4 Changed 3 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 3 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 3 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 3 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: Changed 2 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 10 months ago by vdelecroix

Replying to tscrim:

Is this subsumed by #20214?

yes!

comment:10 Changed 10 months 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 9 months ago by vdelecroix

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

comment:12 Changed 8 months 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.

Note: See TracTickets for help on using tickets.