Opened 5 years ago
Closed 5 years ago
#22398 closed defect (fixed)
LaurentPolynomial_mpair.__init__ modifies input
Reported by:  etn40ff  Owned by:  

Priority:  critical  Milestone:  sage7.6 
Component:  algebra  Keywords:  
Cc:  dkrenn  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  b56984c (Commits, GitHub, GitLab)  Commit:  b56984cba10abdcbfabda76ac98e1e9aee4d353a 
Dependencies:  Stopgaps: 
Description (last modified by )
Creating mutlivariate Laurent polynomials from other mutlivariate Laurent polynomials sometimes fails.
sage: LQ = LaurentPolynomialRing(QQ, 'x0, x1, x2, y0, y1, y2, y3, y4, y5') sage: LZ = LaurentPolynomialRing(ZZ, 'x0, x1, x2, y0, y1, y2, y3, y4, y5') sage: LQ.inject_variables() Defining x0, x1, x2, y0, y1, y2, y3, y4, y5 sage: x2^1*y0*y1*y2*y3*y4*y5 + x1^1*x2^1*y0*y1*y3*y4 + x0^1 in LZ True sage: x2^1*y0*y1*y2*y3*y4*y5 + x1^1*x2^1*y0*y1*y3*y4 + x0^1*x1^1*y0*y3 + x0^1 in LZ Traceback (most recent call last): ... AttributeError: 'tuple' object has no attribute 'esub'
Apparently this is due to the fact that LaurentPolynomial_mpair.__init__
changes the keys of the input dictionary.
Change History (15)
comment:1 Changed 5 years ago by
 Branch set to public/ticket/22398
 Cc dkrenn added
 Commit set to 500205f092692e0d9da392c1a3a0bc8de76cda9e
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Commit changed from 500205f092692e0d9da392c1a3a0bc8de76cda9e to c930e8c451332b36a635742de0451c059dcef8ad
Branch pushed to git repo; I updated commit sha1. New commits:
c930e8c  Trac #22398: minor rewrite to use tuple(...) instead of copy(...)

comment:4 Changed 5 years ago by
 Status changed from new to needs_review
comment:5 followups: ↓ 6 ↓ 8 Changed 5 years ago by
 Priority changed from major to critical
 Status changed from needs_review to needs_work
The correct solution is to not modify the input data (instead of working around the fact that the input data is modified).
It would be good to add an explicit doctest showing that the input is not modified. Something like
sage: LQ.<x,y> = LaurentPolynomialRing(QQ) sage: D = {(1, 1): 1} sage: type(D.keys()[0]) <type 'tuple'> sage: LQ(D) x^1*y sage: type(D.keys()[0]) <type 'tuple'>
comment:6 in reply to: ↑ 5 Changed 5 years ago by
Replying to jdemeyer:
The correct solution is to not modify the input data (instead of working around the fact that the input data is modified).
I completely agree. I'll adapt the patch during the day (once my 7.6.beta4 has recompiled again...).
comment:7 Changed 5 years ago by
 Commit changed from c930e8c451332b36a635742de0451c059dcef8ad to 4a31db664c74f247e2e36a96b940c964c877a7df
comment:8 in reply to: ↑ 5 Changed 5 years ago by
 Status changed from needs_work to needs_review
Replying to jdemeyer:
The correct solution is to not modify the input data (instead of working around the fact that the input data is modified).
Done. Needs_review (...and let's see what the patchbot says).
comment:9 followup: ↓ 11 Changed 5 years ago by
 Status changed from needs_review to needs_work
In the doctests, you can simplify code of the form
id_y = id(y) id(x) == id_y
by
x is y
comment:10 Changed 5 years ago by
 Commit changed from 4a31db664c74f247e2e36a96b940c964c877a7df to 1f2d7f442d649b13d44d013a37348fa194672641
Branch pushed to git repo; I updated commit sha1. New commits:
1f2d7f4  Trac #22398: simplify id(...) = id(...) in doctest

comment:11 in reply to: ↑ 9 Changed 5 years ago by
 Status changed from needs_work to needs_review
Replying to jdemeyer:
In the doctests, you can simplify code of the form
id(x) == id(y)by
x is y
Oh, indeed :) ...done.
comment:12 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
If tests pass, you can set this to positive_review.
comment:13 Changed 5 years ago by
 Commit changed from 1f2d7f442d649b13d44d013a37348fa194672641 to b56984cba10abdcbfabda76ac98e1e9aee4d353a
Branch pushed to git repo; I updated commit sha1. New commits:
b56984c  Trac #22398: py3: fix <type 'tuple'>

comment:14 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:15 Changed 5 years ago by
 Branch changed from public/ticket/22398 to b56984cba10abdcbfabda76ac98e1e9aee4d353a
 Resolution set to fixed
 Status changed from positive_review to closed
Your patch seems to work. Thanks
New commits:
fix for loop whose changing its keys inside
Added trac reference