Opened 17 months ago
Closed 16 months ago
#27457 closed enhancement (fixed)
py3: fixes for isidentifier
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  python3  Keywords:  
Cc:  embray, jdemeyer, tscrim  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Erik Bray 
Report Upstream:  N/A  Work issues:  
Branch:  94899ab (Commits)  Commit:  94899ab9b91354bd5d760c6238423f51e91807f3 
Dependencies:  Stopgaps: 
Description
because the python3 method of the same name is not enough for our needs.
Change History (24)
comment:1 Changed 17 months ago by
 Branch set to u/chapoton/27457
 Commit set to 241168c05e18d98fa04a0d01c25ead49a1695089
 Status changed from new to needs_review
comment:2 Changed 17 months ago by
 Commit changed from 241168c05e18d98fa04a0d01c25ead49a1695089 to db5c8f475aea3cacb6fec1c759fa9ff3ec07f686
Branch pushed to git repo; I updated commit sha1. New commits:
db5c8f4  better fix for isidentified

comment:3 Changed 17 months ago by
 Cc embray jdemeyer tscrim added
green bot, please review. I have a small doubt on what to do exactly..
comment:4 Changed 17 months ago by
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
diff git a/src/sage/symbolic/ring.pyx b/src/sage/symbolic/ring.pyx index f6bd6a8..0a0f7ab 100644
a b cdef class SymbolicRing(CommutativeRing): 841 841 842 842 for s in names_list: 843 843 if not isidentifier(s): 844 raise ValueError('The name "' +s+'" is not a valid Python identifier.')844 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 fstring:
raise ValueError(f'The name "{s}" is not a valid Python identifier.')
comment:5 Changed 17 months ago by
 Commit changed from db5c8f475aea3cacb6fec1c759fa9ff3ec07f686 to 5bab779cd2b7ff02d64e729883a24f0f3578139d
Branch pushed to git repo; I updated commit sha1. New commits:
5bab779  trac 27457 more doc, small code changes

comment:6 Changed 17 months ago by
 Reviewers set to Erik Bray
 Status changed from needs_review to positive_review
comment:7 Changed 17 months ago by
 Branch changed from u/chapoton/27457 to 5bab779cd2b7ff02d64e729883a24f0f3578139d
 Resolution set to fixed
 Status changed from positive_review to closed
comment:8 Changed 17 months ago by
 Commit 5bab779cd2b7ff02d64e729883a24f0f3578139d deleted
 Resolution fixed deleted
 Status changed from closed to new
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/sitepackages/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/sitepackages/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/sitepackages/sage/calculus/calculus.py", line 1766, in inverse_laplace return result._sage_() File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.6/sitepackages/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/sitepackages/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/sitepackages/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]
comment:9 Changed 17 months ago by
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
comment:10 Changed 17 months ago by
How about this:

src/sage/interfaces/sympy.py
diff git a/src/sage/interfaces/sympy.py b/src/sage/interfaces/sympy.py index 2ac9e40b79..f56159fa59 100644
a b def _sympysage_symbol(self): 226 226 sage: assert x == Symbol('x')._sage_() 227 227 """ 228 228 from sage.symbolic.ring import SR 229 return SR.var(self.name) 229 try: 230 return SR.var(self.name) 231 except ValueError: 232 # sympy sometimes returns variables with name = 'None', str rep = '_None' 233 return SR.var(str(self)) 230 234 231 235 def _sympysage_Subs(self): 232 236 """
comment:11 Changed 17 months ago by
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
diff git a/src/sage/symbolic/ring.pyx b/src/sage/symbolic/ring.pyx index df045bd0d0..11f14e2c18 100644
a b from sage.structure.coerce cimport is_numpy_type 31 31 32 32 from sage.rings.all import RR, CC, ZZ 33 33 34 import keyword 34 35 import operator 35 36 import parser 36 37 … … def is_SymbolicVariable(x): 1333 1334 """ 1334 1335 return is_Expression(x) and is_a_symbol((<Expression>x)._gobj) 1335 1336 1337 # Don't allow any of these keywords as identifiers for symbolic variables. 1338 KEYWORDS = set(keyword.kwlist).union(['exec', 'print', 'None', 'True', 1339 'False', 'nonlocal']) 1336 1340 1337 1341 def isidentifier(x): 1338 1342 """ … … def isidentifier(x): 1367 1371 True 1368 1372 sage: isidentifier('lambda s:s+1') 1369 1373 False 1370 sage: isidentifier('None') # py31374 sage: isidentifier('None') 1371 1375 False 1372 1376 sage: isidentifier('lambda') 1373 1377 False 1374 1378 sage: isidentifier('def') 1375 1379 False 1376 1380 """ 1377 import keyword 1378 if keyword.iskeyword(x): 1381 if x in KEYWORDS: 1379 1382 return False 1380 1383 try:
comment:12 Changed 17 months ago by
 Milestone changed from sage8.7 to sage8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:13 Changed 16 months ago by
 Branch changed from 5bab779cd2b7ff02d64e729883a24f0f3578139d to u/chapoton/27457
 Commit set to 5bab779cd2b7ff02d64e729883a24f0f3578139d
comment:14 Changed 16 months ago by
 Commit changed from 5bab779cd2b7ff02d64e729883a24f0f3578139d to 1220ea59e228db35d453b240559d10f4031e217f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1220ea5  py3: fixes for isidentifier

comment:15 Changed 16 months ago by
 Status changed from new to needs_review
comment:16 Changed 16 months ago by
 Status changed from needs_review to needs_work
still need to fix the failure with python3 in src/sage/calculus/calculus.py
comment:17 Changed 16 months ago by
Does the change in comment:10 work and make sense?
comment:18 Changed 16 months ago by
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
.
EDIT: at least now, we have the same failure in py2 and py3..
comment:19 Changed 16 months ago by
So like this?

src/sage/interfaces/sympy.py
diff git a/src/sage/interfaces/sympy.py b/src/sage/interfaces/sympy.py index 2ac9e40b79..cb18cf8ec6 100644
a b def _sympysage_symbol(self): 226 226 sage: assert x == Symbol('x')._sage_() 227 227 """ 228 228 from sage.symbolic.ring import SR 229 return SR.var(self.name) 229 if self.name is not 'None': 230 return SR.var(self.name) 230 231 231 232 def _sympysage_Subs(self): 232 233 """
Plus some documentation and a doctest, if possible.
Or is it a bad idea to specialcase 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.
comment:20 Changed 16 months ago by
 Commit changed from 1220ea59e228db35d453b240559d10f4031e217f to 94899ab9b91354bd5d760c6238423f51e91807f3
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
94899ab  py3: fixes for isidentifier

comment:21 Changed 16 months ago by
 Status changed from needs_work to needs_review
let us try John's first proposal
comment:22 Changed 16 months ago by
bot is green on python2, and tests are ok with python3. I would say this is a reasonable solution, even if not elegant.
comment:23 Changed 16 months ago by
 Status changed from needs_review to positive_review
I seeas long as it works it's fine by me.
comment:24 Changed 16 months ago by
 Branch changed from u/chapoton/27457 to 94899ab9b91354bd5d760c6238423f51e91807f3
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: fixes for isidentifier