Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LaurentPolynomial_mpair.__init__ modifies input #22398

Closed
Etn40ff opened this issue Feb 20, 2017 · 21 comments
Closed

LaurentPolynomial_mpair.__init__ modifies input #22398

Etn40ff opened this issue Feb 20, 2017 · 21 comments

Comments

@Etn40ff
Copy link
Contributor

Etn40ff commented Feb 20, 2017

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.

CC: @dkrenn

Component: algebra

Author: Daniel Krenn

Branch/Commit: b56984c

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/22398

@Etn40ff Etn40ff added this to the sage-7.6 milestone Feb 20, 2017
@Etn40ff
Copy link
Contributor Author

Etn40ff commented Feb 20, 2017

Commit: 500205f

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Feb 20, 2017

comment:1

Your patch seems to work.
Thanks


New commits:

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

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Feb 20, 2017

Branch: public/ticket/22398

@Etn40ff

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 21, 2017

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 21, 2017

Changed commit from 500205f to c930e8c

@dkrenn
Copy link
Contributor

dkrenn commented Feb 21, 2017

Author: Daniel Krenn

@jdemeyer
Copy link

comment:5

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

@dkrenn
Copy link
Contributor

dkrenn commented Feb 21, 2017

comment:6

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...).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 21, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 21, 2017

Changed commit from c930e8c to 4a31db6

@dkrenn
Copy link
Contributor

dkrenn commented Feb 21, 2017

comment:8

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).

@jdemeyer
Copy link

comment:9

In the doctests, you can simplify code of the form

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

by

x is y

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 21, 2017

Changed commit from 4a31db6 to 1f2d7f4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 21, 2017

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

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

@dkrenn
Copy link
Contributor

dkrenn commented Feb 21, 2017

comment:11

Replying to @jdemeyer:

In the doctests, you can simplify code of the form

id(x) == id(y)

by

x is y

Oh, indeed :) ...done.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

comment:12

If tests pass, you can set this to positive_review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2017

Changed commit from 1f2d7f4 to b56984c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2017

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

b56984cTrac #22398: py3: fix

@vbraun
Copy link
Member

vbraun commented Feb 24, 2017

Changed branch from public/ticket/22398 to b56984c

@vbraun vbraun closed this as completed in f4f37b1 Feb 24, 2017
dimpase pushed a commit to tornaria/sage that referenced this issue Feb 7, 2023
…git://trac.sagemath.org/sage into t/22067/gf-polyhedron

* 'public/polynomials/laurent_mpoly_constructor-21976' of git://trac.sagemath.org/sage: (709 commits)
  Some reviewer changes.
  Updated SageMath version to 7.6.beta5
  py3: remove usage of `_richcmp` in complex fields
  a few conversions from _cmp_ to _richcmp_
  Trac sagemath#22404: forgotten change in doctest
  Allow custom CXXFLAGS for pynac
  Mark surf as experimental
  List surf dependencies, do not build by default
  some doc cleanup in ell_rational_field and 2 neighbor files
  Trac sagemath#22398: py3: fix <type 'tuple'>
  trac 21592 some missing accents
  Clean up pynac interface
  py3 compatibility
  Trac sagemath#22404: add blanks after commas (except for tuples representing exponents) in polydict
  Improvements and some cleanup to ETuple.
  Trac sagemath#22396: mention Trac ticket in Tests
  Trac sagemath#22392: Mention Trac Ticket in Tests
  Trac sagemath#22402: PEP8: fix trailing whitespace
  Trac sagemath#22398: simplify id(...) = id(...) in doctest
  Trac sagemath#22398: doctest non-modifying input
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants