Opened 9 months ago

Closed 8 months ago

#27457 closed enhancement (fixed)

py3: fixes for isidentifier

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.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 9 months ago by chapoton

  • Branch set to u/chapoton/27457
  • Commit set to 241168c05e18d98fa04a0d01c25ead49a1695089
  • Status changed from new to needs_review

New commits:

241168cpy3: fixes for isidentifier

comment:2 Changed 9 months ago by git

  • Commit changed from 241168c05e18d98fa04a0d01c25ead49a1695089 to db5c8f475aea3cacb6fec1c759fa9ff3ec07f686

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

db5c8f4better fix for isidentified

comment:3 Changed 9 months ago by chapoton

  • Cc embray jdemeyer tscrim added

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

comment:4 Changed 9 months ago by 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

    diff --git a/src/sage/symbolic/ring.pyx b/src/sage/symbolic/ring.pyx
    index f6bd6a8..0a0f7ab 100644
    a b cdef class SymbolicRing(CommutativeRing): 
    841841
    842842        for s in names_list:
    843843            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 f-string:

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

comment:5 Changed 9 months ago by git

  • Commit changed from db5c8f475aea3cacb6fec1c759fa9ff3ec07f686 to 5bab779cd2b7ff02d64e729883a24f0f3578139d

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

5bab779trac 27457 more doc, small code changes

comment:6 Changed 9 months ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

comment:7 Changed 9 months ago by vbraun

  • Branch changed from u/chapoton/27457 to 5bab779cd2b7ff02d64e729883a24f0f3578139d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:8 Changed 9 months ago by vbraun

  • 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/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]

comment:9 Changed 9 months ago by 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

comment:10 Changed 9 months ago by jhpalmieri

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): 
    226226        sage: assert x == Symbol('x')._sage_()
    227227    """
    228228    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))
    230234
    231235def _sympysage_Subs(self):
    232236     """

comment:11 Changed 9 months ago by 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

    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 
    3131
    3232from sage.rings.all import RR, CC, ZZ
    3333
     34import keyword
    3435import operator
    3536import parser
    3637   
    def is_SymbolicVariable(x): 
    13331334    """
    13341335    return is_Expression(x) and is_a_symbol((<Expression>x)._gobj)
    13351336
     1337# Don't allow any of these keywords as identifiers for symbolic variables.
     1338KEYWORDS = set(keyword.kwlist).union(['exec', 'print', 'None', 'True',
     1339                                      'False', 'nonlocal'])
    13361340
    13371341def isidentifier(x):
    13381342    """
    def isidentifier(x): 
    13671371        True
    13681372        sage: isidentifier('lambda s:s+1')
    13691373        False
    1370         sage: isidentifier('None')  # py3
     1374        sage: isidentifier('None')
    13711375        False
    13721376        sage: isidentifier('lambda')
    13731377        False
    13741378        sage: isidentifier('def')
    13751379        False
    13761380    """
    1377     import keyword
    1378     if keyword.iskeyword(x):
     1381    if x in KEYWORDS:
    13791382        return False
    13801383    try:

comment:12 Changed 9 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.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 9 months ago by chapoton

  • Branch changed from 5bab779cd2b7ff02d64e729883a24f0f3578139d to u/chapoton/27457
  • Commit set to 5bab779cd2b7ff02d64e729883a24f0f3578139d

New commits:

241168cpy3: fixes for isidentifier
db5c8f4better fix for isidentified
5bab779trac 27457 more doc, small code changes

comment:14 Changed 9 months ago by git

  • Commit changed from 5bab779cd2b7ff02d64e729883a24f0f3578139d to 1220ea59e228db35d453b240559d10f4031e217f

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

1220ea5py3: fixes for isidentifier

comment:15 Changed 9 months ago by chapoton

  • Status changed from new to needs_review

comment:16 Changed 9 months ago by chapoton

  • 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 9 months ago by jhpalmieri

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

comment:18 Changed 9 months ago by 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.

Version 0, edited 9 months ago by chapoton (next)

comment:19 Changed 9 months ago by jhpalmieri

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): 
    226226        sage: assert x == Symbol('x')._sage_()
    227227    """
    228228    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)
    230231
    231232def _sympysage_Subs(self):
    232233     """

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.

comment:20 Changed 8 months ago by git

  • Commit changed from 1220ea59e228db35d453b240559d10f4031e217f to 94899ab9b91354bd5d760c6238423f51e91807f3

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

94899abpy3: fixes for isidentifier

comment:21 Changed 8 months ago by chapoton

  • Status changed from needs_work to needs_review

let us try John's first proposal

comment:22 Changed 8 months ago by chapoton

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 8 months ago by embray

  • Status changed from needs_review to positive_review

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

comment:24 Changed 8 months ago by vbraun

  • Branch changed from u/chapoton/27457 to 94899ab9b91354bd5d760c6238423f51e91807f3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.