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

lambdify is no longer able to deal with sympy.vector.BaseScalar #26690

Closed
Davide-sd opened this issue Jun 10, 2024 · 17 comments · Fixed by #26713
Closed

lambdify is no longer able to deal with sympy.vector.BaseScalar #26690

Davide-sd opened this issue Jun 10, 2024 · 17 comments · Fixed by #26713

Comments

@Davide-sd
Copy link
Contributor

Davide-sd commented Jun 10, 2024

Example code:

from sympy import *
from sympy.vector import CoordSys3D, gradient

t = symbols("t")
N = CoordSys3D("N")
i, j, k = N.base_vectors()
xn, yn, zn = N.base_scalars()
expr = -(xn**2) * tan(t) ** 2 + yn**2 + zn**2
g = gradient(expr)
m = g.magnitude()
vec = g / m
components = vec.to_matrix(N)
for i in range(3):
    print(f"component {i}:", components[i])

# component 0: -2*N.x*tan(t)**2/sqrt(4*N.x**2*tan(t)**4 + 4*N.y**2 + 4*N.z**2)
# component 1: 2*N.y/sqrt(4*N.x**2*tan(t)**4 + 4*N.y**2 + 4*N.z**2)
# component 2: 2*N.z/sqrt(4*N.x**2*tan(t)**4 + 4*N.y**2 + 4*N.z**2)

Lambdifying with sympy 1.12.1. Note that instances of BaseScalar are converted to dummy symbols.

f = lambdify([xn, yn, zn, t], components[0])
print(help(f))
Help on function _lambdifygenerated:

_lambdifygenerated(Dummy_41, Dummy_40, Dummy_39, t)
    Created with lambdify. Signature:

    func(N.x, N.y, N.z, t)

    Expression:

    -2*N.x*tan(t)**2/sqrt(4*N.x**2*tan(t)**4 + 4*N.y**2 + 4*N.z**2)

    Source code:

    def _lambdifygenerated(Dummy_41, Dummy_40, Dummy_39, t):
        return -2*Dummy_41*tan(t)**2/sqrt(4*Dummy_39**2 + 4*Dummy_40**2 + 4*Dummy_41**2*tan(t)**4)


    Imported modules:

None

Lambdifying with sympy 1.13 rc1: no longer supported. That was essential to properly plot vector fields with my plotting module.

f = lambdify([xn, yn, zn, t], components[0])
print(help(f))
---------------------------------------------------------------------------
PrintMethodNotImplementedError            Traceback (most recent call last)
Cell In[2], line 1
----> 1 f = lambdify([xn, yn, zn, t], components[0])
      2 print(help(f))

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/utilities/lambdify.py:880](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/utilities/lambdify.py#line=879), in lambdify(args, expr, modules, printer, use_imps, dummify, cse, docstring_limit)
    878 else:
    879     cses, _expr = (), expr
--> 880 funcstr = funcprinter.doprint(funcname, iterable_args, _expr, cses=cses)
    882 # Collect the module imports from the code printers.
    883 imp_mod_lines = []

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/utilities/lambdify.py:1145](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/utilities/lambdify.py#line=1144), in _EvaluatorPrinter.doprint(self, funcname, args, expr, cses)
   1143     cses = zip(subvars, subexprs)
   1144 else:
-> 1145     argstrs, expr = self._preprocess(args, expr)
   1147 # Generate argument unpacking and final argument list
   1148 funcargs = []

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/utilities/lambdify.py:1213](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/utilities/lambdify.py#line=1212), in _EvaluatorPrinter._preprocess(self, args, expr)
   1211     s = str(arg)
   1212 elif isinstance(arg, Basic) and arg.is_symbol:
-> 1213     s = self._argrepr(arg)
   1214     if dummify or not self._is_safe_ident(s):
   1215         dummy = Dummy()

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/printing/codeprinter.py:172](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/printing/codeprinter.py#line=171), in CodePrinter.doprint(self, expr, assign_to)
    169 self._not_supported = set()
    170 self._number_symbols = set()
--> 172 lines = self._print(expr).splitlines()
    174 # format the output
    175 if self._settings["human"]:

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/printing/printer.py:331](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/printing/printer.py#line=330), in Printer._print(self, expr, **kwargs)
    329     printmethod = getattr(self, printmethodname, None)
    330     if printmethod is not None:
--> 331         return printmethod(expr, **kwargs)
    332 # Unknown object, fall back to the emptyPrinter.
    333 return self.emptyPrinter(expr)

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/printing/codeprinter.py:461](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/printing/codeprinter.py#line=460), in CodePrinter._print_Function(self, expr)
    459     return '%s(%s)' % (self._print(expr.func), ', '.join(map(self._print, expr.args)))
    460 else:
--> 461     return self._print_not_supported(expr)

File [~/Documents/Development/envs/plot/lib/python3.12/site-packages/sympy/printing/codeprinter.py:582](http://localhost:8888/envs/plot/lib/python3.12/site-packages/sympy/printing/codeprinter.py#line=581), in CodePrinter._print_not_supported(self, expr)
    580 def _print_not_supported(self, expr):
    581     if self._settings.get('strict', False):
--> 582         raise PrintMethodNotImplementedError("Unsupported by %s: %s" % (str(type(self)), str(type(expr))) + \
    583                          "\nSet the printer option 'strict' to False in order to generate partially printed code.")
    584     try:
    585         self._not_supported.add(expr)

PrintMethodNotImplementedError: Unsupported by <class 'sympy.printing.lambdarepr.LambdaPrinter'>: <class 'sympy.vector.scalar.BaseScalar'>
Set the printer option 'strict' to False in order to generate partially printed code.
@moorepants
Copy link
Member

Likely the same fundamental issue reported at #26663

@bjodah
Copy link
Member

bjodah commented Jun 10, 2024

I think this issue is different enough to merit its own issue. I will see if I can fix this separately (looking at this quickly I think I see a way).

bjodah added a commit to bjodah/sympy that referenced this issue Jun 10, 2024
@bjodah
Copy link
Member

bjodah commented Jun 10, 2024

I added a commit to gh-26678 (and test) which I think fixes this issue.

@oscarbenjamin
Copy link
Collaborator

I bisected the regression here to 82f80c0 from gh-25913.

I think that strict=True is the better print option here including for lambdify so setting strict=False as in 25a9fb0 does not seem like the right fix to me. The printer should recognise that the base scalars are the symbols that are the arguments i.e. it doesn't need to print anything for base scalar types.

@oscarbenjamin
Copy link
Collaborator

Perhaps lambdify should replace the arguments with Dummy symbols at the outset before calling the code printer.

@bjodah
Copy link
Member

bjodah commented Jun 11, 2024

Somehow it does (I find it challenging to grasp the control flow in lambdify.py), the code that gets compiled uses dummy symbols, but the arguments in the doc-string has the invalid identifiers.

@oscarbenjamin
Copy link
Collaborator

It tries to print the arguments here:

argstrs, expr = self._preprocess(args, expr)

@bjodah
Copy link
Member

bjodah commented Jun 11, 2024

Sorry I was vague: I know, I have stepped through this, that's how I came up with adding {'strict': False} only to the printer stored in _EvaluatorPrinter._argrepr. I think it does the right thing here, but there are many branches so my confidence would have been higher if the code wasn't as complex as it is...

@oscarbenjamin
Copy link
Collaborator

The problem is that this code tries to print the argument first before deciding to use a Dummy:

def _preprocess(self, args, expr):
"""Preprocess args, expr to replace arguments that do not map
to valid Python identifiers.
Returns string form of args, and updated expr.
"""
from sympy.core.basic import Basic
from sympy.core.sorting import ordered
from sympy.core.function import (Derivative, Function)
from sympy.core.symbol import Dummy, uniquely_named_symbol
from sympy.matrices import DeferredVector
from sympy.core.expr import Expr
# Args of type Dummy can cause name collisions with args
# of type Symbol. Force dummify of everything in this
# situation.
dummify = self._dummify or any(
isinstance(arg, Dummy) for arg in flatten(args))
argstrs = [None]*len(args)
for arg, i in reversed(list(ordered(zip(args, range(len(args)))))):
if iterable(arg):
s, expr = self._preprocess(arg, expr)
elif isinstance(arg, DeferredVector):
s = str(arg)
elif isinstance(arg, Basic) and arg.is_symbol:
s = self._argrepr(arg)
if dummify or not self._is_safe_ident(s):
dummy = Dummy()
if isinstance(expr, Expr):
dummy = uniquely_named_symbol(
dummy.name, expr, modify=lambda s: '_' + s)
s = self._argrepr(dummy)
expr = self._subexpr(expr, {arg: dummy})
elif dummify or isinstance(arg, (Function, Derivative)):
dummy = Dummy()
s = self._argrepr(dummy)
expr = self._subexpr(expr, {arg: dummy})
else:
s = str(arg)
argstrs[i] = s
return argstrs, expr

That whole function should be rewritten in a simpler more robust way. The different special cases should not be needed. They mostly amount to the same thing which is just that it might be necessary to replace the arg with a Dummy. There should be no need to special case DeferredVector, Function, Derivative. Probably the is_symbol check should be replaced with is_Symbol.

@bjodah
Copy link
Member

bjodah commented Jun 11, 2024

Agreed. Although I will not find the time in the near future for any ambitious rewrite.

@oscarbenjamin
Copy link
Collaborator

Probably DeferredVector should be removed entirely. I don't see what its purpose is.

@oscarbenjamin
Copy link
Collaborator

This is a quick fix:

diff --git a/sympy/utilities/lambdify.py b/sympy/utilities/lambdify.py
index be06906291..f1602508e7 100644
--- a/sympy/utilities/lambdify.py
+++ b/sympy/utilities/lambdify.py
@@ -1210,8 +1210,12 @@ def _preprocess(self, args, expr):
             elif isinstance(arg, DeferredVector):
                 s = str(arg)
             elif isinstance(arg, Basic) and arg.is_symbol:
-                s = self._argrepr(arg)
-                if dummify or not self._is_safe_ident(s):
+                s = None
+                if not dummify and arg.is_Symbol:
+                    s = self._argrepr(arg)
+                    if not self._is_safe_ident(s):
+                        s = None
+                if s is None:
                     dummy = Dummy()
                     if isinstance(expr, Expr):
                         dummy = uniquely_named_symbol(

@moorepants
Copy link
Member

Probably DeferredVector should be removed entirely. I don't see what its purpose is.

It used to be necessary to use this so you could provide an index-able argument to your lambdified function, but it is no longer needed because x = [a, b]; lambdify((x,), expr) works (long back it didn't).

@oscarbenjamin
Copy link
Collaborator

This is a quick fix:

Unfortunately this fails some tests where a MatrixSymbol is the parameter and the expectation is to pass a numpy array as the argument.

An even simpler fix then is just:

diff --git a/sympy/utilities/lambdify.py b/sympy/utilities/lambdify.py
index be06906291..18383bf640 100644
--- a/sympy/utilities/lambdify.py
+++ b/sympy/utilities/lambdify.py
@@ -1210,7 +1210,7 @@ def _preprocess(self, args, expr):
             elif isinstance(arg, DeferredVector):
                 s = str(arg)
             elif isinstance(arg, Basic) and arg.is_symbol:
-                s = self._argrepr(arg)
+                s = str(arg)
                 if dummify or not self._is_safe_ident(s):
                     dummy = Dummy()
                     if isinstance(expr, Expr):

@oscarbenjamin
Copy link
Collaborator

We should keep this open until the fix is confirmed in 1.13rc2.

oscarbenjamin added a commit that referenced this issue Jun 16, 2024
@oscarbenjamin
Copy link
Collaborator

@Davide-sd I have just pushed SymPy 1.13.0rc2 to PyPI which should have the fix for this issue.

Can you test with it?

@Davide-sd
Copy link
Contributor Author

Run the tests, everything works fine now. Thanks everyone, I'm closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants