core/containers: no longer create Tuple's with lists as arguments. #22551

ThePauliPrinciple · 2021-11-26T01:10:18Z

References to other Issues or PRs

Brief description of what is fixed or changed

Several classes in SymPy use sympify on their arguments. In some cases, this leads to the creation of Tuple objects that have lists as their arguments. This PR solves those cases as far as they show up in the tests, by explicitly looping over iterables when a mix of tuple and list can be used for the input arguments.

Other comments

Release Notes

NO ENTRY

sympy-bot · 2021-11-26T01:10:27Z

✅

Hi, I am the SymPy bot (v162). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

No release notes entry will be added for this pull request.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #22550

#### Brief description of what is fixed or changed
Several classes in SymPy use `sympify` on their arguments. In some cases, this leads to the creation of `Tuple` objects that have `lists` as their arguments. This PR solves those cases as far as they show up in the tests, by explicitly looping over iterables when a mix of `tuple` and `list` can be used for the input arguments.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

…gument

ThePauliPrinciple · 2021-11-29T12:19:02Z

sympy/core/containers.py

@@ -50,6 +50,12 @@ class Tuple(Basic):
    def __new__(cls, *args, **kwargs):
        if kwargs.get('sympify', True):
            args = (sympify(arg) for arg in args)
+        #if kwargs.get('denest_lists', False):


This is to be removed before merging, but shows that currently there are no longer any Tuple with list args being created during the testing.

ThePauliPrinciple · 2021-11-29T12:20:54Z

sympy/core/function.py

@@ -1270,19 +1270,20 @@ def __new__(cls, expr, *variables, **kwargs):
                        must be supplied to differentiate %s''' % expr))

        # Standardize the variables by sympifying them:
-        variables = list(sympify(variables))
+        #variables = sympify(list(variables))


This is actually not needed here since all variables are eventually stored in Tuple (or Array which again stores it in Tuple) where sympify is also used.

Maybe this should just be deleted.

ThePauliPrinciple · 2021-11-29T12:29:38Z

sympy/printing/tests/test_julia.py

@@ -212,7 +212,8 @@ def test_containers():
    assert julia_code(Tuple(*[1, 2, 3])) == "(1, 2, 3)"
    assert julia_code((1, x*y, (3, x**2))) == "(1, x.*y, (3, x.^2))"
    # scalar, matrix, empty matrix and empty list
-    assert julia_code((1, eye(3), Matrix(0, 0, []), [])) == "(1, [1 0 0;\n0 1 0;\n0 0 1], zeros(0, 0), Any[])"
+    from sympy.codegen.pynodes import List


Should there be sympy/codegen/{julia,maple,octave}nodes.py files defining List or perhaps this means that List should be moved to a more general file in sympy/codegen. Obviously the import should be made top-level but I put it here for convenience of review.

I'm not sure that List has the same meaning in all languages or if this should be considered an abstract "list" that would have different implementations in different languages.

I think each code printer has its own way to print a list, but since sympify is used on the input to CodePrinters, this line causes the creation of a Tuple, which then has a list in it. Changing this to a List seems to avoid this problem, I'm not sure there is a better way. List is actually just a wrapper around Tuple which gets printed differently (if I understand correctly, it defaults to printing the same as list.

I think you know more about this than I do so I can't really advise but I trust your judgement.

@bjodah do you have any preference for how this is dealt with?

I'm fine with moving it. I put it in pynodes to signal equivalence with Python lists.

ThePauliPrinciple · 2021-11-29T12:30:35Z

sympy/simplify/tests/test_cse.py

@@ -584,8 +584,10 @@ def test_cse_list():
    assert _cse(x) == ([], x)
    assert _cse('x') == ([], 'x')
    it = [x]
-    for c in (list, tuple, set, Tuple):
+    for c in (list, tuple, set):


Tuple doesn't do typecasting, the equivalent of tuple(x) is Tuple(*x).

github-actions · 2021-11-29T14:05:13Z

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [907895ac]       [4489b4c7]
-      4.23±0.03s         323±20ms     0.08  polygon.PolygonArbitraryPoint.time_bench01
+      3.45±0.2ms       5.78±0.3ms     1.67  solve.TimeMatrixOperations.time_det(4, 2)
+      3.22±0.2ms      6.02±0.06ms     1.87  solve.TimeMatrixOperations.time_det_bareiss(4, 2)
+        37.5±1ms         56.5±2ms     1.51  solve.TimeMatrixSolvePyDySlow.time_linsolve(1)
+      38.4±0.9ms         57.7±3ms     1.50  solve.TimeMatrixSolvePyDySlow.time_solve(1)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

oscarbenjamin · 2021-11-29T15:42:59Z

sympy/core/containers.py

@@ -50,6 +50,12 @@ class Tuple(Basic):
    def __new__(cls, *args, **kwargs):


Probably this should use an explicit keyword argument:

def __new__(cls, *args, sympify=False):

Would it be confusing if this gets changed in this PR? Since it solves a different issue. Note also that the current default value is sympify=True.

Turns out that this is not trival. Dict assumes Tuple uses sympify and not _sympify (to convert str to Symbols).

So if you use sympify as a keyword argument, you don't have access to it anymore

oscarbenjamin · 2021-11-29T15:46:02Z

sympy/core/containers.py

+        args = tuple(args)
+        for arg in args:
+            if isinstance(arg, list):
+                warnings.warn(DeprecationWarning('Tuple',arg))


Deprecation warnings in SymPy are usually given like this:

sympy/sympy/core/sympify.py

Lines 453 to 460 in ed0bfd1

SymPyDeprecationWarning(

feature="String fallback in sympify",

useinstead= \

'sympify(str(obj)) or ' + \

'sympy.core.sympify.converter or obj._sympy_',

issue=18066,

deprecated_since_version='1.6'

).warn()

The SymPyDeprecationWarning class is handled specially by the test runner so that it is an error if it is emitted in the test suite.

As far as I am concerned, this warning will be removed before the merge (see also the first review comment by me). If such a warning is to be added, it should be added to Basic when all such issues are resolved. This is for demonstration purposes. (that there are no longer such violations)

sympy-bot · 2021-11-30T20:32:03Z

🟠

Hi, I am the SymPy bot (v162). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it.

This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75.

The following commits add new files:

2e98db8:
- sympy/codegen/abstract_nodes.py

If these files were added/deleted on purpose, you can ignore this message.

oscarbenjamin · 2021-12-01T23:01:35Z

Is this finished now?

ThePauliPrinciple · 2021-12-02T01:13:16Z

@bjodah Do you have an opinion about the new location of List? I also noticed that CodePrinter actually uses Lists (regardless of language), but only after creating Tuples first.

I think it is possible to have sympify use extra rules, so it might be better to have CodePrinter already convert lists to Lists inside sympify so that the current tests do not have to be changed.

Is abstract_nodes good name for the new file? Or would container_nodes be better?

bjodah

I'm fine with putting the class in .abstract_nodes.

bjodah · 2021-12-02T08:25:50Z

sympy/printing/tests/test_julia.py

@@ -212,7 +213,7 @@ def test_containers():
    assert julia_code(Tuple(*[1, 2, 3])) == "(1, 2, 3)"
    assert julia_code((1, x*y, (3, x**2))) == "(1, x.*y, (3, x.^2))"
    # scalar, matrix, empty matrix and empty list
-    assert julia_code((1, eye(3), Matrix(0, 0, []), [])) == "(1, [1 0 0;\n0 1 0;\n0 0 1], zeros(0, 0), Any[])"


this looks like it introduces backwards incompatibility? If so, I'm afraid it needs a deprecation cycle.

bjodah · 2021-12-02T08:26:22Z

sympy/printing/tests/test_maple.py

@@ -233,8 +234,7 @@ def test_containers():
    assert maple_code(Tuple(*[1, 2, 3])) == "[1, 2, 3]"
    assert maple_code((1, x * y, (3, x ** 2))) == "[1, x*y, [3, x^2]]"
    # scalar, matrix, empty matrix and empty list
-
-    assert maple_code((1, eye(3), Matrix(0, 0, []), [])) == \
+    assert maple_code((1, eye(3), Matrix(0, 0, []), List(*[]))) == \


same thing here and for the octave test too.

ThePauliPrinciple · 2021-12-02T12:11:45Z

sympy/printing/codeprinter.py

+        #for codeprinting, all lists should be converted to abstract_nodes.Lists.
+        from sympy.codegen.abstract_nodes import List
+        store_list_converter=converter.get(list, None)
+        converter[list]=lambda x: List(*x)


I'm adding list->List conversion to sympify for the purposes of CodePrinting, this will allow current tuple/list nesting without creating lists in Tuples but not change anything for the user. Is there a better way to do this?

This kind of coding is fragile in my humble opinion: importing a mutable object, modifying it in-place, calling a function using it, and then reverting the change. If the converter in .sympify is intended to be modifiable, then I'd rather see a context manager class in .sympify allowing to override the default converters for a region of code, would you agree?

This is also not thread-safe. The converters are only really supposed to be registered during start up.

Probably somewhere there should explicit code that checks for list and converts to List rather than depending on sympify to do this implicitly.

Point taken, I'll try to see of sympy.strategies can be of any help.

I think this has been solved.

ThePauliPrinciple · 2021-12-02T12:12:51Z

sympy/printing/codeprinter.py

+            del converter[list]
+        else:
+            converter[list] = store_list_converter
+


Restore sympify to normal behaviour. Does it make sense to add this as an option to symplify?

bjodah · 2021-12-02T17:15:39Z

sympy/printing/codeprinter.py

        expr = _handle_assign_to(expr, assign_to)
-        expr = _convert_python_lists(expr)


This is where the explicit conversion was done previously. (Note that this was added only for backwards compatibility, I would have rather done without it, but it was already being called with [] as can be found in the tests).

It needs to happen before _handle_assign_to (since it will gladly convert ([],) to Tuple([]) in sympify which should not be allowed).

can we simply move the call up?

Currently _convert_python_lists only goes through lists (so it would stop as soon as it sees the tuple). I could try to change it to loop through tuple too.

I updated _convert_python_lists to recurse through tuple and moved it forward. I believe this solves all issues.

bjodah · 2021-12-02T20:36:12Z

Thanks! The changes regarding List now looks good to me 🙂👍

oscarbenjamin · 2021-12-02T21:12:05Z

sympy/codegen/abstract_nodes.py

+
+class List(Tuple):
+    """Represents a (frozen) (Python) list (for code printing purposes)."""
+    def __eq__(self, other):


Is the __eq__ method needed for this class to function or is it just for the tests?

In general it's better if Basic subclass don't override __eq__.

from sympy import * class List(Tuple): pass print([1] == List(1)) class List(Tuple): """Represents a (frozen) (Python) list (for code printing purposes).""" def __eq__(self, other): if isinstance(other, list): return self == List(*other) else: return self.args == other print([1] == List(1))

Shows that the first case is False while the second case is True

Yes, but why does that matter? Does the List class actually need to be compared (with ==) to the list class in order to function properly in actual use?

I'm not sure, that would have to be tested. Note that I only moved this class. It already had this functionality, so I assume there was a reason (perhaps a bit naive). It was previously in pynodes.py, which should indicate that this is specific to python printers, which it actually is not since it was already being used for all CodePrinters. The 'bug' related to this class was actually that it had to be used earlier in CodePrinter.doprint to avoid converting Tuples with lists as arguments.

Note that I only moved this class

Oh, okay. Let's just leave it for now then.

I think we can remove the __eq__ method and the test if we want. I didn't reflect over implications of overriding __eq__ in a Basic subclass.

oscarbenjamin · 2021-12-02T21:56:20Z

Looks good. Thanks!

core/containers: raising error when lists are used as arguments in Tuple

04a36de

ThePauliPrinciple added 6 commits November 26, 2021 13:45

create tuple of comprehension so it can be evaluated multipel times

76b7776

recursively convert list arguments to Tuple into Tuples

c2f6c9e

added tests for Tuple(list) cases

adb3f5f

removed unused import

7ac1aef

updated changed doctest result

8bbcd8f

removed misuses of sympify that would create Tuples with non-Basic ar…

626064e

…gument

ThePauliPrinciple commented Nov 29, 2021

View reviewed changes

ThePauliPrinciple added 2 commits November 29, 2021 13:22

removed new test that is no longer required

2611600

removed trailing whitespaces

9ed6df0

ThePauliPrinciple commented Nov 29, 2021

View reviewed changes

restored previous version of the doctest

d5748ba

oscarbenjamin reviewed Nov 29, 2021

View reviewed changes

ThePauliPrinciple changed the title ~~core/containers: raising error when lists are used as arguments in Tuple~~ core/containers: no longer create Tuple's with lists as arguments. Nov 30, 2021

made the use of codegen.List more general

2e98db8

ThePauliPrinciple added 3 commits November 30, 2021 21:40

removed warnings

c106c7e

solved name issue with List

e8ba012

removed commented code

40193d9

ThePauliPrinciple mentioned this pull request Dec 1, 2021

Deprecate the use of non-Basic #22445

Open

oscarbenjamin added codegen core labels Dec 1, 2021

bjodah requested changes Dec 2, 2021

View reviewed changes

enhanced List conversion in codeprinter

cf86440

ThePauliPrinciple commented Dec 2, 2021

View reviewed changes

ThePauliPrinciple added 2 commits December 2, 2021 13:15

added converter import

f4c01b1

restored removed empty line

5769049

oscarbenjamin mentioned this pull request Dec 2, 2021

has_free defined and used to solve issues with non-Symbol generators #22539

Merged

ThePauliPrinciple requested a review from bjodah December 2, 2021 15:52

bjodah reviewed Dec 2, 2021

View reviewed changes

ThePauliPrinciple added 3 commits December 2, 2021 18:46

converting lists before handling assignments

3eee47e

removed unneeded import

b5e37cc

removed unneccesary line deletion

41ba53c

bjodah approved these changes Dec 2, 2021

View reviewed changes

oscarbenjamin reviewed Dec 2, 2021

View reviewed changes

oscarbenjamin merged commit 95f0228 into sympy:master Dec 2, 2021

ThePauliPrinciple deleted the tuple_list_argument branch December 2, 2021 21:58

oscarbenjamin mentioned this pull request Feb 22, 2022

Derivative, ArrayDerivative, MatrixExpr: Accept numbers.Integral as order of differentiation, index #23145

Closed

ThePauliPrinciple mentioned this pull request Mar 9, 2022

diff raises SympifyError since sympy version 1.10 #23222

Closed

		@@ -50,6 +50,12 @@ class Tuple(Basic):
		def __new__(cls, args, *kwargs):

	SymPyDeprecationWarning(
	feature="String fallback in sympify",
	useinstead= \
	'sympify(str(obj)) or ' + \
	'sympy.core.sympify.converter or obj._sympy_',
	issue=18066,
	deprecated_since_version='1.6'
	).warn()

		expr = _handle_assign_to(expr, assign_to)
		expr = _convert_python_lists(expr)

core/containers: no longer create Tuple's with lists as arguments. #22551

core/containers: no longer create Tuple's with lists as arguments. #22551

Conversation

ThePauliPrinciple commented Nov 26, 2021 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Nov 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThePauliPrinciple Nov 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThePauliPrinciple Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sympy-bot commented Nov 30, 2021 • edited

🟠

oscarbenjamin commented Dec 1, 2021

ThePauliPrinciple commented Dec 2, 2021

bjodah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjodah Dec 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjodah commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjodah Dec 3, 2021 • edited

Choose a reason for hiding this comment

oscarbenjamin commented Dec 2, 2021

ThePauliPrinciple commented Nov 26, 2021 •

edited

sympy-bot commented Nov 26, 2021 •

edited

ThePauliPrinciple Nov 29, 2021 •

edited

github-actions bot commented Nov 29, 2021 •

edited

ThePauliPrinciple Nov 30, 2021 •

edited

sympy-bot commented Nov 30, 2021 •

edited

bjodah Dec 2, 2021 •

edited

bjodah Dec 3, 2021 •

edited