Opened 6 years ago

Closed 6 years ago

#22398 closed defect (fixed)

LaurentPolynomial_mpair.__init__ modifies input

Reported by: Salvatore Stella Owned by:
Priority: critical Milestone: sage-7.6
Component: algebra Keywords:
Cc: Daniel Krenn Merged in:
Authors: Daniel Krenn Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: b56984c (Commits, GitHub, GitLab) Commit: b56984cba10abdcbfabda76ac98e1e9aee4d353a
Dependencies: Stopgaps:

Status badges

Description (last modified by Salvatore Stella)

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 Salvatore Stella

Branch: public/ticket/22398
Cc: Daniel Krenn added
Commit: 500205f092692e0d9da392c1a3a0bc8de76cda9e

Your patch seems to work. Thanks


New commits:

ba66e9cfix for loop whose changing its keys inside
500205fAdded trac reference

comment:2 Changed 6 years ago by Salvatore Stella

Description: modified (diff)

comment:3 Changed 6 years ago by git

Commit: 500205f092692e0d9da392c1a3a0bc8de76cda9ec930e8c451332b36a635742de0451c059dcef8ad

Branch pushed to git repo; I updated commit sha1. New commits:

c930e8cTrac #22398: minor rewrite to use tuple(...) instead of copy(...)

comment:4 Changed 6 years ago by Daniel Krenn

Authors: Daniel Krenn
Status: newneeds_review

comment:5 Changed 6 years ago by Jeroen Demeyer

Priority: majorcritical
Status: needs_reviewneeds_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 6 years ago by Daniel Krenn

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 git

Commit: c930e8c451332b36a635742de0451c059dcef8ad4a31db664c74f247e2e36a96b940c964c877a7df

Branch pushed to git repo; I updated commit sha1. New commits:

a4e1504Trac #22398: make __init__ not modify its input
1ca887aTrac #22398: simplify factoring out _mon
4a31db6Trac #22398: doctest non-modifying input

comment:8 in reply to:  5 Changed 6 years ago by Daniel Krenn

Status: needs_workneeds_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 Changed 6 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

In the doctests, you can simplify code of the form

id_y = id(y)
id(x) == id_y

by

x is y
Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:10 Changed 6 years ago by git

Commit: 4a31db664c74f247e2e36a96b940c964c877a7df1f2d7f442d649b13d44d013a37348fa194672641

Branch pushed to git repo; I updated commit sha1. New commits:

1f2d7f4Trac #22398: simplify id(...) = id(...) in doctest

comment:11 in reply to:  9 Changed 6 years ago by Daniel Krenn

Status: needs_workneeds_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 Jeroen Demeyer

Reviewers: Jeroen Demeyer

If tests pass, you can set this to positive_review.

comment:13 Changed 6 years ago by git

Commit: 1f2d7f442d649b13d44d013a37348fa194672641b56984cba10abdcbfabda76ac98e1e9aee4d353a

Branch pushed to git repo; I updated commit sha1. New commits:

b56984cTrac #22398: py3: fix <type 'tuple'>

comment:14 Changed 6 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

comment:15 Changed 6 years ago by Volker Braun

Branch: public/ticket/22398b56984cba10abdcbfabda76ac98e1e9aee4d353a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.