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: 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:

Status badges

Description (last modified by etn40ff)

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 etn40ff

  • Branch set to public/ticket/22398
  • Cc dkrenn added
  • Commit set to 500205f092692e0d9da392c1a3a0bc8de76cda9e

Your patch seems to work. Thanks


New commits:

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

comment:2 Changed 5 years ago by etn40ff

  • Description modified (diff)

comment:3 Changed 5 years ago by git

  • Commit changed from 500205f092692e0d9da392c1a3a0bc8de76cda9e to c930e8c451332b36a635742de0451c059dcef8ad

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

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

comment:4 Changed 5 years ago by dkrenn

  • Authors set to Daniel Krenn
  • Status changed from new to needs_review

comment:5 follow-ups: Changed 5 years ago by jdemeyer

  • 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 dkrenn

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 git

  • Commit changed from c930e8c451332b36a635742de0451c059dcef8ad to 4a31db664c74f247e2e36a96b940c964c877a7df

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 5 years ago by dkrenn

  • 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 follow-up: Changed 5 years ago by jdemeyer

  • 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
Last edited 5 years ago by jdemeyer (previous) (diff)

comment:10 Changed 5 years ago by git

  • Commit changed from 4a31db664c74f247e2e36a96b940c964c877a7df to 1f2d7f442d649b13d44d013a37348fa194672641

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 5 years ago by dkrenn

  • 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 jdemeyer

  • Reviewers set to Jeroen Demeyer

If tests pass, you can set this to positive_review.

comment:13 Changed 5 years ago by git

  • Commit changed from 1f2d7f442d649b13d44d013a37348fa194672641 to b56984cba10abdcbfabda76ac98e1e9aee4d353a

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

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

comment:14 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun

  • Branch changed from public/ticket/22398 to b56984cba10abdcbfabda76ac98e1e9aee4d353a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.