sympy icon indicating copy to clipboard operation
sympy copied to clipboard

Lambdify doesn't recognize derivative symbol if cse is enabled

Open tjstienstra opened this issue 2 years ago • 12 comments

Here is a minimal reproducer:

>>> import sympy as sm   
>>> t = sm.symbols("t")
>>> x = sm.Function("x")(t)
>>> xd = x.diff(t)
>>> sm.lambdify((xd, x), xd + x)(1, 1)
2
>>> sm.lambdify((xd, x), xd, cse=True)(1, 1)     
1
>>> sm.lambdify((xd, x), xd + x, cse=True)(1, 1) 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "...\sympy\utilities\lambdify.py", line 875, in lambdify
    funcstr = funcprinter.doprint(funcname, iterable_args, _expr, cses=cses)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\sympy\utilities\lambdify.py", line 1166, in doprint
    str_expr = _recursive_to_string(self._exprrepr, expr)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\sympy\utilities\lambdify.py", line 961, in _recursive_to_string
    return doprint(arg)
           ^^^^^^^^^^^^
  File "...\sympy\printing\codeprinter.py", line 172, in doprint
    lines = self._print(expr).splitlines()
            ^^^^^^^^^^^^^^^^^
  File "...\sympy\printing\printer.py", line 331, in _print
    return printmethod(expr, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\sympy\printing\str.py", line 57, in _print_Add
    t = self._print(term)
        ^^^^^^^^^^^^^^^^^
  File "...\sympy\printing\printer.py", line 331, in _print
    return printmethod(expr, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\sympy\printing\codeprinter.py", line 582, in _print_not_supported
    raise PrintMethodNotImplementedError("Unsupported by %s: %s" % (str(type(self)), str(type(expr))) + \
sympy.printing.codeprinter.PrintMethodNotImplementedError: Unsupported by <class 'sympy.printing.numpy.SciPyPrinter'>: <class 'sympy.core.function.Derivative'>
Set the printer option 'strict' to False in order to generate partially printed code.

I am not sure if this issue is easily solvable as the problem seems to be that it tries to print the expression x0 + Derivative(x0, t) with the [(x0, x(t))] as subexpressions.

tjstienstra avatar Mar 25 '24 16:03 tjstienstra

I thought this would work, but it doesn't:

In [1]: import sympy as sm

In [2]: t = sm.symbols("t")

In [3]: x = sm.Function("x")(t)

In [4]: xd = x.diff(t)

In [5]: sm.lambdify((xd, x), xd + x, cse=True, dummify=True)(1, 1)
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
Cell In[5], line 1
----> 1 sm.lambdify((xd, x), xd + x, cse=True, dummify=True)(1, 1) 

File <lambdifygenerated-1>:5, in _lambdifygenerated(_Dummy_36, _Dummy_37)
      1 def _lambdifygenerated(_Dummy_36, _Dummy_37):
      2     x0 = _Dummy_37
      3     return (  # Not supported in Python with SciPy and NumPy:
      4   # Derivative
----> 5 x0 + Derivative(x0, t))

NameError: name 'Derivative' is not defined

moorepants avatar Mar 25 '24 19:03 moorepants

cse is called on the expressions and then the cse results are passed to the printer which runs a preprocessor on the results that do the dummifying. I'm guessing those operations should be swapped for things to work correctly.

moorepants avatar Mar 26 '24 04:03 moorepants

Hello,

I've looked a bit into the code, I think the issue is specific to using xd = x.diff(t). This case is not tested(referring to test_lambdify.py) and with the current implementation, the Dummy_XY are not placed correctly in the expression calculated by cse since you would need to look recursively and replace recursively in the Derivative(x0, t) when x0 is replaced with a dummy (although looking recursively might not be needed if we could implement a simple way to link them) .

I'll look into it more and indeed, if we call cse after the printer it works perfectly, but then the results are not used.

mleila1312 avatar Apr 03 '24 21:04 mleila1312

The dummy replacement works recursively already. Maybe just look into having the preprocessor apply the dummy subs to all of the cse output.

moorepants avatar Apr 04 '24 03:04 moorepants

Actually this code looks like the preprocessor is applied to all outputs of cse:

https://github.com/sympy/sympy/blob/master/sympy/utilities/lambdify.py#L1133-L1140

moorepants avatar Apr 04 '24 03:04 moorepants

It looks like in the CSE process, which happens first, the argument of the derivative gets swapped out:

ipdb> expr
[x0 + Derivative(x0, t), x(t)]

This would then cause the dummy replacement to be missed because it is looking for Derivative(x, t) (probably). So making the dummy replacement happen first might be better.

In [3]: x + xd
Out[3]: x(t) + Derivative(x(t), t)

In [4]: sm.cse(x + xd)
Out[4]: ([(x0, x(t))], [x0 + Derivative(x0, t)])

moorepants avatar Apr 04 '24 03:04 moorepants

One option would be a flag to cse() to have it skip Derivative(x, t) type terms.

moorepants avatar Apr 04 '24 03:04 moorepants

Maybe a pre and post optimization could be used for cse, something like:

In [27]: def pre(expr): return expr.xreplace({xd: sm.Symbol('a')})

In [28]: def post(expr): return expr.xreplace({sm.Symbol('a'): xd})

In [29]: sm.cse(x + xd, optimizations=[(pre, post)])
Out[29]: ([], [x(t) + Derivative(x(t), t)])

In [31]: sm.cse(x + x*x + xd, optimizations=[(pre, post)])
Out[31]: ([(x0, x(t))], [x0**2 + x0 + Derivative(x(t), t)])

moorepants avatar Apr 04 '24 04:04 moorepants

This would then cause the dummy replacement to be missed because it is looking for Derivative(x, t) (probably).

That's what I ended up thinking to

One option would be a flag to cse() to have it skip Derivative(x, t) type terms.

A flag could work, I was also thinking that we could consider the derivative (Derivative(x0, t)) like a replacement but I think the flag would be more practical.

Maybe a pre and post optimization could be used for cse

It would work to, but what are the advantages to this compared to the flag possibility?(since with this possibility, we would go trhough the expression twice and still ignore the derivatives)

mleila1312 avatar Apr 04 '24 07:04 mleila1312

It would work to, but what are the advantages to this compared to the flag possibility?(since with this possibility, we would go trhough the expression twice and still ignore the derivatives)

It is a flag that is already present, so we would not need to implement a new kwarg in cse.

moorepants avatar Apr 04 '24 08:04 moorepants

It is a flag that is already present, so we would not need to implement a new kwarg in cse.

But isn't there some cases where we want the arguments of the derivative also changed accordingly?( instead of Derivative(x,t), we would want Derivative(x0,t))

I've been trying to implement the change(not using pre-prost method but a flad indicating cse is called inside lambdify), but I'm running into a difficulty and I don't know how to resolve it.

I've added an optionnal parameter in cse that indicates that it is called inside a lambdify function. When I simply used it as a non-optionnal parameter (put right after expr), the function worked perfectly and the bug was resolved. However, when put as an optionnal parameter, the value of flag_lambdify(name of the flag) isn't modified, and I've noticed that it is the same for the optionnal parameter list.

To see this problem, add the new flag variable, add a print(list, flag_lambdify) under the header of the function cse and enter the lines of code above that produced the bug. The print is also executed 2 times( you'll have a first print with the right values, and a second one with the default values).

If you want, you can also see the modified files in my sympy repo(although I'm not sure I left the print(list, flag_lambdify) when I pushed).

Also, I've not put the flag_lamdbify as a non-optionnal parameter because I think it is used in other files, but if it is not it would solve the problem( but not the problem with the list variable)

mleila1312 avatar Apr 04 '24 10:04 mleila1312

This seems to be fixed in master:

moorepants@nandi:sympy(master)$ ipython
Python 3.11.9 | packaged by conda-forge | (main, Apr 19 2024, 18:36:13) [GCC 12.3.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.26.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: %paste
>>> import sympy as sm   
>>> t = sm.symbols("t")
>>> x = sm.Function("x")(t)
>>> xd = x.diff(t)
>>> sm.lambdify((xd, x), xd + x)(1, 1)

## -- End pasted text --
Out[1]: 2

In [2]: sm.lambdify((xd, x), xd, cse=True)(1, 1)
Out[2]: 1

In [3]: sm.lambdify((xd, x), xd + x, cse=True)(1, 1)
Out[3]: 2

moorepants avatar Aug 27 '24 10:08 moorepants

test script:

import sympy as sm
t = sm.symbols("t")
x = sm.Function("x")(t)
xd = x.diff(t)
print(sm.lambdify((xd, x), xd + x)(1, 1))
print(sm.lambdify((xd, x), xd, cse=True)(1, 1))
print(sm.lambdify((xd, x), xd + x, cse=True)(1, 1))
print(sm.lambdify((xd, x), xd + x, cse=True, dummify=True)(1, 1))

moorepants avatar Aug 27 '24 11:08 moorepants

I tried to git bisect this against a version tag, e.g. sympy-1.13.0 and master but I get this error:

Some good revs are not ancestors of the bad rev.
git bisect cannot work properly in this case.
Maybe you mistook good and bad revs?

moorepants avatar Aug 27 '24 11:08 moorepants

Closing as fixed, not quite sure what PR fixed it though.

moorepants avatar Aug 27 '24 11:08 moorepants

I tried to git bisect this against a version tag, e.g. sympy-1.13.0 and master but I get this error:

It is definitely possible to bisect between a version tag and master. You might need to do something like git fetch upstream to make sure you have all the commits/tags:

$ git checkout master
$ git bisect bad
You need to start by "git bisect start"

Do you want me to do it for you [Y/n]? 
$ git checkout sympy-1.13.0
...
HEAD is now at 5f58c25d47 Merge pull request #26763 from oscarbenjamin/pr_bump_versions_1.13.0_final
$ git bisect good
Bisecting: 361 revisions left to test after this (roughly 9 steps)
[0d2f438a4af34e1595194acd5d76a017e82519af] Update test_actuator.py

I asked in the other issue whether this is a regression in 1.13 and whether it warrants a backport to a 1.13.3 release. Obviously we can't backport if we don't know what PR fixed it though.

oscarbenjamin avatar Aug 27 '24 11:08 oscarbenjamin

I tried this:

moorepants@nandi:sympy(master)$ git pull upstream --tags
remote: Enumerating objects: 133, done.
remote: Counting objects: 100% (133/133), done.
remote: Compressing objects: 100% (56/56), done.
remote: Total 133 (delta 79), reused 107 (delta 75), pack-reused 0 (from 0)
Receiving objects: 100% (133/133), 78.12 KiB | 930.00 KiB/s, done.
Resolving deltas: 100% (79/79), completed with 26 local objects.
From github.com:sympy/sympy
   06bc81eafc..2197797741  1.13               -> upstream/1.13
 * [new branch]            revert-26358-26338 -> upstream/revert-26358-26338
 * [new tag]               1.13.2             -> 1.13.2
 * [new tag]               sympy-1.13.1       -> sympy-1.13.1
 * [new tag]               sympy-1.13.2       -> sympy-1.13.2
You asked to pull from the remote 'upstream', but did not specify
a branch. Because this is not the default configured remote
for your current branch, you must specify a branch on the command line.
moorepants@nandi:sympy(master)$ git checkout sympy-1.12
sympy-1.12        sympy-1.12.1      sympy-1.12.1rc1   sympy-1.12rc1     
moorepants@nandi:sympy(master)$ git checkout sympy-1.12.1
Note: switching to 'sympy-1.12.1'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at a36a8b23de Merge pull request #26635 from oscarbenjamin/pr_ci_update
moorepants@nandi:sympy((sympy-1.12.1))$ python lam_diff.py 
2
1
Traceback (most recent call last):
  File "/home/moorepants/src/sympy/lam_diff.py", line 7, in <module>
    print(sm.lambdify((xd, x), xd + x, cse=True)(1, 1))
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<lambdifygenerated-3>", line 5, in _lambdifygenerated
NameError: name 'Derivative' is not defined
moorepants@nandi:sympy((sympy-1.12.1))$ git bisect start
status: waiting for both good and bad commits
moorepants@nandi:sympy((sympy-1.12.1)|BISECTING)$ git bisect bad
status: waiting for good commit(s), bad commit known
moorepants@nandi:sympy((sympy-1.12.1)|BISECTING)$ git checkout master
Previous HEAD position was a36a8b23de Merge pull request #26635 from oscarbenjamin/pr_ci_update
Switched to branch 'master'
Your branch is ahead of 'origin/master' by 2162 commits.
  (use "git push" to publish your local commits)
moorepants@nandi:sympy(master|BISECTING)$ git bisect good
Some good revs are not ancestors of the bad rev.
git bisect cannot work properly in this case.
Maybe you mistook good and bad revs?

moorepants avatar Aug 27 '24 11:08 moorepants

Okay, you can't use git bisect like that.

There is no way with git bisect to say that you are looking for a fix rather than a break: you have to say that a newer commit is the bad one and an older commit is the good one. Then you have to invert the logic in your test script so that it asserts that the bug is not fixed like:

try:
   func()
except Exception:
   print('failed')
else:
   assert False, "did not fail"

oscarbenjamin avatar Aug 27 '24 11:08 oscarbenjamin

Hence the error message from git:

Some good revs are not ancestors of the bad rev. git bisect cannot work properly in this case. Maybe you mistook good and bad revs?

oscarbenjamin avatar Aug 27 '24 11:08 oscarbenjamin

Thanks, I guess I've never tried this inversion because I'm usually looking for where something broke.

moorepants avatar Aug 27 '24 11:08 moorepants

#26678 seems to have fixed this issue.

tjstienstra avatar Aug 27 '24 11:08 tjstienstra