py3: fixes for isidentifier

[chapoton]

because the python3 method of the same name is not enough for our needs.

[chapoton]

New commits:

​241168c

py3: fixes for isidentifier

[git]

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

​db5c8f4

better fix for isidentified

[chapoton]

green bot, please review. I have a small doubt on what to do exactly..

[embray]

because the python3 method of the same name is not enough for our needs.

Is there some reference for this? I believe it, I just don't know what the explanation is. Perhaps the text removed from the documentation could instead be changed to explain why this function is needed and the builtin one from Python is not sufficient.

When making trivial changes like

src/sage/symbolic/ring.pyx

@@ -841 +841 @@
 
         for s in names_list:
             if not isidentifier(s):
-                raise ValueError('The name "'+s+'" is not a valid Python identifier.')
+                raise ValueError('The name "' + s + '" is not a valid Python identifier.')

I think we might as well change it to use normal string formatting instead of pointless concatenations. Since this is a Cython module you can use an f-string:

               raise ValueError(f'The name "{s}" is not a valid Python identifier.')

[git]

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

​5bab779

trac 27457 more doc, small code changes

[vbraun]

On Python3:

sage -t --long src/sage/calculus/calculus.py
**********************************************************************
File "src/sage/calculus/calculus.py", line 1745, in sage.calculus.calculus.inverse_laplace
Failed example:
    inverse_laplace(cos(s), s, t, algorithm='sympy')
Exception raised:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.calculus.calculus.inverse_laplace[21]>", line 1, in <module>
        inverse_laplace(cos(s), s, t, algorithm='sympy')
      File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/site-packages/sage/calculus/calculus.py", line 1766, in inverse_laplace
        return result._sage_()
      File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/site-packages/sage/interfaces/sympy.py", line 275, in _sympysage_function
        args = [arg._sage_() for arg in self.args]
      File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/site-packages/sage/interfaces/sympy.py", line 275, in <listcomp>
        args = [arg._sage_() for arg in self.args]
      File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/site-packages/sage/interfaces/sympy.py", line 229, in _sympysage_symbol
        return SR.var(self.name)
      File "sage/symbolic/ring.pyx", line 844, in sage.symbolic.ring.SymbolicRing.var (build/cythonized/sage/symbolic/ring.cpp:9725)
        raise ValueError(f'The name "{s}" is not a valid Python identifier.')
    ValueError: The name "None" is not a valid Python identifier.
**********************************************************************
1 item had failures:
   1 of  24 in sage.calculus.calculus.inverse_laplace
    [418 tests, 1 failure, 17.42 s]

[chapoton]

Hmm. Sympy send us back a variable _None whose name is 'None'. What should we do with that ? Make a special case to map it to None ?

sage: var('t, s')
sage: ex=cos(s)
sage: ex_sy, s, t = [expr._sympy_() for expr in (ex, s, t)]
....: from sympy import inverse_laplace_transform
....: from sage.interfaces.sympy import sympy_init
....: sympy_init()
....: result = inverse_laplace_transform(ex_sy, s, t)
sage: d = result.args[3]; d

[jhpalmieri]

How about this:

src/sage/interfaces/sympy.py

@@ -226 +226 @@
         sage: assert x == Symbol('x')._sage_()
     """
     from sage.symbolic.ring import SR
-    return SR.var(self.name)
+    try:
+        return SR.var(self.name)
+    except ValueError:
+        # sympy sometimes returns variables with name = 'None', str rep = '_None'
+        return SR.var(str(self))
 
 def _sympysage_Subs(self):
      """

[jhpalmieri]

I also wonder if we should also forbid the use of 'None', 'True', 'False' as variable names in Python 2. Differences between the lists of keywords in Python 2 vs. Python 3:

• in keywords.kwlist in Python 2 but not 3: 'exec', 'print'
• in keywords.kwlist in Python 3 but not 2: 'None', 'True', 'False', 'nonlocal'

Should Sage maintain its own list of forbidden variable names, for example the union of the two versions of keywords.kwlist? Something like this:

src/sage/symbolic/ring.pyx

@@ -31 +31 @@
 
 from sage.rings.all import RR, CC, ZZ
 
+import keyword
 import operator
 import parser
     
@@ -1333 +1334 @@
     """
     return is_Expression(x) and is_a_symbol((<Expression>x)._gobj)
 
+# Don't allow any of these keywords as identifiers for symbolic variables.
+KEYWORDS = set(keyword.kwlist).union(['exec', 'print', 'None', 'True',
+                                      'False', 'nonlocal'])
 
 def isidentifier(x):
     """
@@ -1367 +1371 @@
         True
         sage: isidentifier('lambda s:s+1')
         False
-        sage: isidentifier('None')  # py3
+        sage: isidentifier('None')
         False
         sage: isidentifier('lambda')
         False
         sage: isidentifier('def')
         False
     """
-    import keyword
-    if keyword.iskeyword(x):
+    if x in KEYWORDS:
         return False
     try:

[embray]

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

[chapoton]

New commits:

​241168c	py3: fixes for isidentifier
​db5c8f4	better fix for isidentified
​5bab779	trac 27457 more doc, small code changes

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​1220ea5

py3: fixes for isidentifier

[chapoton]

still need to fix the failure with python3 in src/sage/calculus/calculus.py

[jhpalmieri]

Does the change in comment:10 work and make sense?

[chapoton]

I am not so sure about your comment:10. When sympy send us None, we should probably not make a symbolic variable out of it, but understand it as None.

[jhpalmieri]

So like this?

src/sage/interfaces/sympy.py

@@ -226 +226 @@
         sage: assert x == Symbol('x')._sage_()
     """
     from sage.symbolic.ring import SR
-    return SR.var(self.name)
+    if self.name is not 'None':
+        return SR.var(self.name)
 
 def _sympysage_Subs(self):
      """

Plus some documentation and a doctest, if possible.

Or is it a bad idea to special-case None like this? I don't know sympy, so I don't know how to (for example) convert the result of its inverse Laplace transform to remove the _None variable.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​94899ab

py3: fixes for isidentifier

[chapoton]

let us try John's first proposal

[chapoton]

bot is green on python2, and tests are ok with python3. I would say this is a reasonable solution, even if not elegant.

[embray]

I see--as long as it works it's fine by me.