subs dict result changed (and inconsistent) in 1.6 with functions

[cmbant]

This code

from sympy import Function
from sympy.abc import t

x, y = Function('x')(t), Function('y')(t)
(x*y).subs({x: x + 1, y: x})

produces the expected result (x(t) + 1)*x(t) with 1.5.1 and earlier, but with 1.6rc1 produces (x(t) + 1)**2. Note that this is even inconsistent with substituting in the ordered dict items order (I believe dict substitution should be independent of order and not make any re-substitutions). I don't see anything in the change log that could justify this behaviour, so assuming a bug (and a tricky one to isolate, took me half a day to figure out why a complex unit test was failing with 1.6rc1). If dict substitute is unsupported (it does seem to be undocumented) then it should raise an error, though not sure if there's another way to do this kind of variable substitution consistently.

[cmbant]

Incidentally, respite being rc, 1.6rc1 seems to be pulled by default from Travis python 3.6 ubuntu build, e..g see https://travis-ci.org/github/cmbant/CAMB/jobs/687446750

[smichr]

See here and containing issue.

[smichr]

But this is interesting. I am noting (and have been frustrated in #19261 by) similar behavior that I was not able to track down. But the replacement order here should be x then y according to the docstring. And if so, then something about the default_sort_key, I am guessing, has changed since op count and nodes for two symbols is the same so the replacements should be sorted according to the default_sort_key ordering of x and y.

Marked as wrong because I believe something is wrong here.

[oscarbenjamin]

I'm not sure what should happen here but I think you should use simultaneous=True if you don't want the substitutions to interfere with each other:

(x*y).subs({x: x + 1, y: x}, simultaneous=True)

You can also pass the substitutions as an ordered iterable:

(x*y).subs({x: x + 1, y: x}.items())

I don't think we can guarantee to match dict insertion order while still supporting Python 3.5.

[oscarbenjamin]

It's hard to tell from the Travis script how exactly sympy is being installed. Maybe it's conda that picks up on prereleases from pypi. Maybe it depends on some of other conda commands that are being run.

[cmbant]

Thanks, yes, simultaneous=True is the workaround for the intended use (I think previously use with dict implied simultaneous?).

Note that symbols give a different result, which is very unintuitive

from sympy.abc import a, b
(a*b).subs({a: a + 1, b: a})

gives a(1+1).

[oscarbenjamin]

I don't know if passing a dict previously implied simultaneous=True but the docstring describes what is expected to happen:

sympy/sympy/core/basic.py

Lines 800 to 804 in 94fb720

	o a dict or set whose key/value items correspond to old/new pairs.
	In this case the old/new pairs will be sorted by op count and in
	case of a tie, by number of args and the default_sort_key. The
	resulting sorted list is then processed as an iterable container
	(see previous).

It seems that @smichr thinks that behaviour is not being respected in this case. I suspect that the specification in the docstring does not match what you would want (in general) anyway though.

[asmeurer]

I checked conda and it there are no conda packages built for 1.6 in the main or conda-forge channels.

I also checked pip install sympy in a fresh environment and it installs 1.5.1. pip install --pre sympy does install 1.6rc1, as expected.

[asmeurer]

Maybe it has something to do with the fact that you use setup.py install. I wasn't able to run setup.py install on CAMB because I couldn't get all the Fortran stuff installed. I'm not clear why setup.py would install sympy pre-releases unless you have it configured to do so somehow.

[oscarbenjamin]

The subs issue here is a release blocker for 1.6.

@smichr is there anything that needs to be fixed for subs?

I've bisected the change to 132acff from #18043

To me the order is just undefined if a dict is passed in. It could be possible to use a topological sort but how do we even know what order the user wants?

Looking at the example in this issue

(x*y).subs({x: x + 1, y: x})

if I saw this code then I would not know what the intention was. If the intention is dict order then we can't use that because it is not deterministic for all supported Python versions.

In sympy 1.5.1 which substitution happens depends on the name of the symbols:

In [1]: (x*y).subs({x:x+1, y:x})                                                                                                  
Out[1]: x⋅(x + 1)

In [2]: (x*y).subs({y:y+1, x:y})                                                                                                  
Out[2]: 
       2
(y + 1) 

[smichr]

To me the order is just undefined if a dict is passed in

Yes, it's unordered so subs has to do something canonical (and as quicly as possible) with it. So the items are sorted: reverse ordered non-atoms followed by ordered atoms. The problem/difference is that the sorting of non-atoms -- in this case x(t) and y(t) -- if there is a tie of node length then they should be ordered.

from sympy.core.compatibility import _nodes
# order so more complex items are first and items
# of identical complexity are ordered so f(x) < f(y) < x < y
k = list(ordered(sequence))
# sort is stable so equal _node expressions are still ordered
k.sort(key=lambda x: -_nodes(x))
sequence = [(k, sequence[k]) for k in k]

I can send a PR this evening (or can review anyone else's before then).

[cmbant]

Regarding why 1.6 is installed, I'm not sure. This was not a conda build, the dependency of the package being installed is just sympy>=1; however the install log gives

Searching for sympy>=1.0
Reading https://pypi.org/simple/sympy/
Downloading https://files.pythonhosted.org/packages/4a/72/ad5bdd0a3c18e9bfc0ed532d0749729bb4b2813251c04d6b737df13e90c4/sympy-1.6rc1-py3-none-any.whl#sha256=d6d0c4990b9789699264e2db70979666289bf48c13fccc6e30aad3063374864d
Best match: sympy 1.6rc1

My unchanged travis tests on python 3.6 ubuntu (and only that one) just suddenly stopped working around the 14th May due to it changing to pulling 1.6rc1.

[asmeurer]

I'm not clear why setup.py install would install a pre-release for SymPy. These sorts of issues are why I have avoided uploading pre-releases to PyPI in the past.

[oscarbenjamin]

I'm reopening this because it is only fixed in the 1.6 branch and not master

[oscarbenjamin]

This was fixed by c1fed2c in #19261