Opened 3 years ago
Closed 5 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 )
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
- 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
- Branch set to u/mmarco/bug_in_laurentpolynomial_factor
comment:3 Changed 3 years ago by
- Commit set to 653446429df8f6187b4de63ef6fad9642408bc93
comment:4 Changed 3 years ago by
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
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
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 isZZ
)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
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 2 years ago by
- 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 6 months ago by
comment:10 Changed 6 months ago by
- 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 6 months ago by
- 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 5 months ago by
- Resolution set to invalid
- Status changed from positive_review to closed
Presuming these are all correctly reviewed as either duplicate, invalid, or wontfix.
Related to #20214. Wouldn't be better that the unit of the factorization actually belong to the ring?
New commits:
Treat case where base ring is not a field