Opened 6 years ago
Closed 6 years ago
#22398 closed defect (fixed)
LaurentPolynomial_mpair.__init__ modifies input
Reported by: | etn40ff | Owned by: | |
---|---|---|---|
Priority: | critical | Milestone: | sage-7.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 6 years ago by
Branch: | → public/ticket/22398 |
---|---|
Cc: | dkrenn added |
Commit: | → 500205f092692e0d9da392c1a3a0bc8de76cda9e |
comment:2 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 6 years ago by
Commit: | 500205f092692e0d9da392c1a3a0bc8de76cda9e → 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 6 years ago by
Authors: | → Daniel Krenn |
---|---|
Status: | new → needs_review |
comment:5 follow-ups: 6 8 Changed 6 years ago by
Priority: | major → critical |
---|---|
Status: | needs_review → 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 Changed 6 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 6 years ago by
Commit: | c930e8c451332b36a635742de0451c059dcef8ad → 4a31db664c74f247e2e36a96b940c964c877a7df |
---|
comment:8 Changed 6 years ago by
Status: | needs_work → 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 follow-up: 11 Changed 6 years ago by
Status: | needs_review → needs_work |
---|
In the doctests, you can simplify code of the form
id(x) == id(y)
by
x is y
comment:10 Changed 6 years ago by
Commit: | 4a31db664c74f247e2e36a96b940c964c877a7df → 1f2d7f442d649b13d44d013a37348fa194672641 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
1f2d7f4 | Trac #22398: simplify id(...) = id(...) in doctest
|
comment:11 Changed 6 years ago by
Status: | needs_work → 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 6 years ago by
Reviewers: | → Jeroen Demeyer |
---|
If tests pass, you can set this to positive_review.
comment:13 Changed 6 years ago by
Commit: | 1f2d7f442d649b13d44d013a37348fa194672641 → b56984cba10abdcbfabda76ac98e1e9aee4d353a |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
b56984c | Trac #22398: py3: fix <type 'tuple'>
|
comment:14 Changed 6 years ago by
Status: | needs_review → positive_review |
---|
comment:15 Changed 6 years ago by
Branch: | public/ticket/22398 → b56984cba10abdcbfabda76ac98e1e9aee4d353a |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Your patch seems to work. Thanks
New commits:
fix for loop whose changing its keys inside
Added trac reference