Opened 7 years ago
Closed 7 years ago
#14017 closed defect (fixed)
Determine the correct argspec for python functions defined in cython files
Reported by: | SimonKing | Owned by: | jason |
---|---|---|---|
Priority: | major | Milestone: | sage-5.7 |
Component: | misc | Keywords: | introspection cython argspec |
Cc: | Merged in: | sage-5.7.beta4 | |
Authors: | Simon King | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
In the following simple example, sage_getargspec does not correctly determine the arguments of a python function:
sage: cython(""" ....: def f_cy(*args,**kwds): ....: pass ....: """) sage: sage_getargspec(f_cy) ArgSpec(args=['*args', '**kwds'], varargs=None, keywords=None, defaults=None)
Writing the same in Python, one gets
sage: def f_py(*args,**kwds): ....: pass ....: sage: sage_getargspec(f_py) ArgSpec(args=[], varargs='args', keywords='kwds', defaults=None)
This bug is what currently prevents as to put UniqueRepresentation
into a cython file.
Attachments (1)
Change History (24)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
Actually a part of the current doctests expect wrong answers:
sage: from sage.misc.sageinspect import _sage_getargspec_cython as sgc # That is ok: sage: sgc("cpdef double abc(self, x=None, base=0):") (['self', 'x', 'base'], None, None, (None, 0)) sage: sgc("def __init__(self, x=None, unsigned int base=0):") (['self', 'x', 'base'], None, None, (None, 0)) # The following is wrong, and is the main reason why we couldnt' simply # put UniqueRepresentation into a Cython file to make it faster: sage: sgc('def o(p, *q, r={}, **s) except? -1:') (['p', '*q', 'r', '**s'], None, None, ({},)) # That is ok: sage: sgc('cpdef how(r=(None, "u:doing?")):') (['r'], None, None, ((None, 'u:doing?'),)) sage: sgc('def _(x="):"):') (['x'], None, None, ('):',)) sage: sgc('def f(z = {(1,2,3): True}):\n return z') (['z'], None, None, ({(1, 2, 3): True},)) # The following gives a valid function definition in a cython file, # hence, there must be no error when determining the argspec sage: sgc('def f(double x, z = {(1,2,3): True}):\n return z') Traceback (most recent call last): ... ValueError: Could not parse cython argspec
Last night I have coded something which I think is a finite state automaton that correctly parses any argspec, provided that the given input is valid in Cython. Hence, I do not claim that my code would detect all possible syntax errors; but if there is no syntax error, then the output should be the correct argspec.
In the above doctests, my code would give the correct results. But I need some more tests at this point, and also I am not sure whether I should try to detect some more syntactical problems.
For example, I currently have the reasonable answer
sage: sgc('def o(int * bla p, *q, r={}, **s) except? -1:') (['p', 'r'], 'q', 's', ['{}'])
(here, int * bla
is thought of as a type definition in Cython, which is of course wrong but doesn't matter), but I also get something that could easily be avoided:
sage: sgc('def o(int * bla p, *q, r={}, , **s) except? -1:') (['p', 'r', 's'], 'q', None, ['{}'])
(here, , **
is thought of as a type definition in Cython, but of course it would be straight forward to recognise that there is one illegal comma inserted).
comment:3 Changed 7 years ago by
I just realise that there is one more wrong expected result in the current doctests:
sage: sgc('def _(x="):"):') (['x'], None, None, ('):',))
The function specification states that the string representation of the default values is returned. But of course '):'
is not the string representation of "):"
. Instead, we should have
sage: sgc('def _(x="):"):') (['x'], None, None, ['"):"'])
and this is actually done by my experimental code.
comment:4 Changed 7 years ago by
Here is some other doctest that currently has a very odd result. The docs say that "In the case of a class or a class instance, the ArgSpec?
of the
new
or
init
methods are returned". The example is
sage: P.<x,y> = QQ[] sage: sage_getargspec(P) ArgSpec(args=['self', 'element'], varargs=None, keywords=None, defaults=None) sage: sage_getargspec(P.__class__) ArgSpec(args=['self', 'element'], varargs=None, keywords=None, defaults=None)
but in fact we have
sage: sage_getargspec(P.__init__) ArgSpec(args=['self', 'base_ring', 'n', 'names', 'order'], varargs=None, keywords=None, defaults=("'degrevlex'",))
and thus the expected answer looks suspicious to me.
comment:5 Changed 7 years ago by
Probably it is better to have the defaults not as string, because _sage_getargspec_from_ast
doesn't return strings either.
comment:6 follow-up: ↓ 8 Changed 7 years ago by
Here is another example I am currently not happy with:
sage: cython(""" def test(): return 1 def f(x,y={test():2},**kwds):pass """ ) sage: sage_getargspec(f) ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({None: 2},))
Hence, the default for 'y' is given as {None: 2}
, but in fact it should be {1: 2}
.
As much as I know, Cython does not make the default values available via attributes of a function, unlike Python. And even if one would try to find the actual value by evaluating the string '{test():2}'
in the module's name space, things could go terribly wrong.
Is someone reading this? How should we proceed in this situation?
comment:7 Changed 7 years ago by
- Status changed from new to needs_review
At sage-devel, Robert Bradshaw confirmed that it is probably best to pre-process the source code of a cython function definition, by letting _sage_getargspec_cython
first remove the argument's type definition and then call _sage_getargspec_from_ast
.
In addition to that, I think for consistency _sage_getargspec_cython
should return an ArgSpec
and not a plain tuple.
Needs review!
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 9 Changed 7 years ago by
Hey Simon,
Replying to SimonKing:
Here is another example I am currently not happy with:
sage: cython(""" def test(): return 1 def f(x,y={test():2},**kwds):pass """ ) sage: sage_getargspec(f) ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({None: 2},))Hence, the default for 'y' is given as
{None: 2}
, but in fact it should be{1: 2}
.As much as I know, Cython does not make the default values available via attributes of a function, unlike Python. And even if one would try to find the actual value by evaluating the string
'{test():2}'
in the module's name space, things could go terribly wrong.Is someone reading this? How should we proceed in this situation?
I need a few questions answered before I can have a reasonable answer:
- When are the argspec functions called, during compile, and roughly by what?
- Would this be caught as an error by the cython compiler (or any other bad syntax for that matter)?
- Would
_sage_getargspec_from_ast
would parse it correctly?
As a general design note, I would think all sage_getargspec*
functions should return the same data in the same format (in regard to your comment here).
As for the patch, two quick things. Is there any reason to have that large commented out code block? Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be raise ExceptionType("msg")
.
Thank you,
Travis
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 7 years ago by
Hi Travis,
Replying to tscrim:
- When are the argspec functions called, during compile, and roughly by what?
The argspec functions have of course nothing to do with compilation. Their main use cases seem to be:
- Creation of the docs. If you look into the Sage reference, you will see that any function or method shows its arguments. I think the documentation would not change with my patch, because from the point of view of documentation, it simply does not matter whether the method
def foo(self, int n, *args): ...
has an argument name*args
(including the*
!!) or has varargs namedargs
(ecluding the*
). - Decorators. The reason for creating this patch is the cached_method decorator. Let foodec be obtained by applying the cached_method decorator to a method foo. When calling foodec, it is needed to determine the names of the arguments of foo, including varargs and keywords. This is done only once, of course.
- Would this be caught as an error by the cython compiler (or any other bad syntax for that matter)?
What would be caught as an error? It really seems you are confusing some things. The compiler did its work already. sage_getargspec (or inspect.getargspec) is called on functions or methods. In the case of Python functions, inspect.getargspec can determine the arguments of a function by certain attributes that the function provides.
But in the case of a Cython function (def or cpdef), these attributes do not exist. Hence, sage_getargspec will try to find the source code (via sage_getsource) and determine the argument list from the source.
Before my patch, the parsing process used to fail on typical function definitions in Cython -- as shown in the ticket description. With my patch, the arguments are correctly determined from the source code.
Relation with the compiler:
Let s be a string, which is some code supposed to define a function.
If _sage_getargspec_cython(s)
raises a syntax error, then Cython is supposed to refuse compilation of s. Hence, _sage_getargspec_cython(s)
will never raise an error on the source code of a successfully compiled function (otherwise it would be a bug).
- Would
_sage_getargspec_from_ast
would parse it correctly?
Cython code can look like "def foo(b, int a=1, *args)"
. _sage_getargspec_from_ast
would not know what to do with that string. As I have explained in some post above, the new _sage_getargspec_cython
will strip the type definitions; hence, it would first transform it into "def dummy(b,a=1,*args)"
and then call _sage_getargspec_from_ast
.
As a general design note, I would think all
sage_getargspec*
functions should return the same data in the same format (in regard to your comment here).
That's what my patch does.
As for the patch, two quick things. Is there any reason to have that large commented out code block?
Oops, sorry, I'll remove that block and will submit a new patch after dinner.
Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be
raise ExceptionType("msg")
.
Never heard of that rule. Could you ask on sage-devel whether this rule should be encouraged?
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 7 years ago by
- Reviewers set to Travis Scrimshaw
Hey Simon,
Replying to SimonKing:
Replying to tscrim:
- When are the argspec functions called, during compile, and roughly by what?
[...] But in the case of a Cython function (def or cpdef), these attributes do not exist. Hence, sage_getargspec will try to find the source code (via sage_getsource) and determine the argument list from the source.
Before my patch, the parsing process used to fail on typical function definitions in Cython -- as shown in the ticket description. With my patch, the arguments are correctly determined from the source code.
Ah I see. That clarifies things for me. Thank you.
Relation with the compiler:
Let s be a string, which is some code supposed to define a function.
If
_sage_getargspec_cython(s)
raises a syntax error, then Cython is supposed to refuse compilation of s. Hence,_sage_getargspec_cython(s)
will never raise an error on the source code of a successfully compiled function (otherwise it would be a bug).
So it's not a requirement to actually check bad syntax, but only needs to guarantee that it will parse correct syntax. Correct?
- Would
_sage_getargspec_from_ast
would parse it correctly?Cython code can look like
"def foo(b, int a=1, *args)"
._sage_getargspec_from_ast
would not know what to do with that string. As I have explained in some post above, the new_sage_getargspec_cython
will strip the type definitions; hence, it would first transform it into"def dummy(b,a=1,*args)"
and then call_sage_getargspec_from_ast
.
Sorry I was unclear; I was thinking of if the function was in python:
sage: def test(): return 1 ....: sage: def f(x,y={test():2},**kwds): pass ....: sage: sage_getargspec(f) ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({1: 2},))
I think I'm now starting I understand what the issue is. However I don't see how evaluating the default argument string in the modules namespace could cause an issue unless the user directly calls _sage_getargspec_cython()
as you are in the tests. (I guess there is also some difficulty in parsing out functions foo(args)
and replacing it with module.foo(args)
...)
As a general design note, I would think all
sage_getargspec*
functions should return the same data in the same format (in regard to your comment here).That's what my patch does.
:D
As for the patch, two quick things. Is there any reason to have that large commented out code block?
Oops, sorry, I'll remove that block and will submit a new patch after dinner.
Thanks.
Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be
raise ExceptionType("msg")
.Never heard of that rule. Could you ask on sage-devel whether this rule should be encouraged?
Here's the topic: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/P35X-WjDuK4
Thank you for clarifying things and answering all of my (crazy/misguided) questions,
Travis
Edit - The reason for the drop in coverage is because sage -coverage
doesn't correctly parse that the functions in the doctests are not "real" functions.
comment:11 in reply to: ↑ 10 Changed 7 years ago by
Hi Travis,
Replying to tscrim:
So it's not a requirement to actually check bad syntax, but only needs to guarantee that it will parse correct syntax. Correct?
Correct. The patch fixes cases in which this used not to be the case. But I think it is a bug if correct syntax is not parsed.
Sorry I was unclear; I was thinking of if the function was in python:
sage: def test(): return 1 ....: sage: def f(x,y={test():2},**kwds): pass ....: sage: sage_getargspec(f) ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({1: 2},))
That's indeed tricky.
This example does work for Python functions, because inspect.getargspec can read off the default values from some attribute of the function -- hence, reading source code is not involved for Python.
But if you put the same definition in a .pyx file, then inspect.getargspec does not work, unless one compiles everything with the "binding" option (which is probably bad for performance). Hence, source code parsing probably is the only way to find the arguments. But as soon as you use external functions to define default values, source code parsing must fail sooner or later.
In this example, _sage_getargspec_from_ast
(and thus _sage_getargspec_cython
) would return {None: 2}
and not {1: 2}
as default value.
I see no safe way to overcome this limitation. But note that knowing the correct default values is less critical than knowing the argument names. The bug that made me create this ticket was: Currently the argument names are not correctly determined, in some situations.
I think I'm now starting I understand what the issue is. However I don't see how evaluating the default argument string in the modules namespace could cause an issue
No, in examples of that kind, source code parsing must fail at some point. Two examples:
- If test() in the example above is a cdef function that is not declared in the .pxd file, then
_sage_getargspec_from_ast
(which returns the incorrect default) can not import it. Hence, it would be impossible to simply calltest()
and see what it returns. - It is easy to write a function whose return value changes over time. If
test()
is such function, then the value used to define the default argument of f would differ from the value deduced by_sage_getargspec_from_ast
.
As for the patch, two quick things. Is there any reason to have that large commented out code block?
Oops, sorry, I'll remove that block and will submit a new patch after dinner.
Thanks.
This will be my next post - dinner was quite long...
Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be
raise ExceptionType("msg")
.Never heard of that rule. Could you ask on sage-devel whether this rule should be encouraged?
Here's the topic: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/P35X-WjDuK4
Thanks!
comment:12 Changed 7 years ago by
The patch is updated - the uncommented chunk of code is removed, and I raise Exception("message").
comment:13 Changed 7 years ago by
- Status changed from needs_review to positive_review
Looks good to me now. Thank you Simon.
Best,
Travis
comment:14 follow-up: ↓ 15 Changed 7 years ago by
- Status changed from positive_review to needs_work
Building the documentation fails badly:
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_element_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_element_letterplace.poly_reduce: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_element_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_element_letterplace.singular_system: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_letterplace.poly_reduce: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_letterplace.singular_system: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/letterplace_ideal.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.letterplace_ideal.poly_reduce: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/letterplace_ideal.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.letterplace_ideal.singular_system: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.problem_name: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_lp: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_mps: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.print_ranges: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.problem_name: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_lp: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_mps: Pointer types not allowed in def or cpdef functions /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/rings/polynomial/multi_polynomial_ring_generic.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.multi_polynomial_ring_generic.MPolynomialRing_generic.remove_var: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/rings/polynomial/pbori.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.pbori.BooleanPolynomialRing.remove_var: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/rings/polynomial/pbori.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.pbori.mod_mon_set: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/structure/sage_object.rst:11: WARNING: error while formatting signature for sage.structure.sage_object.load: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/symbolic/expression.rst:11: WARNING: error while formatting signature for sage.symbolic.expression.Expression.add: invalid syntax (<unknown>, line 1) /release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/symbolic/expression.rst:11: WARNING: error while formatting signature for sage.symbolic.expression.Expression.mul: invalid syntax (<unknown>, line 1)
comment:15 in reply to: ↑ 14 Changed 7 years ago by
Replying to jdemeyer:
Building the documentation fails badly:
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_element_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_element_letterplace.poly_reduce: invalid syntax (<unknown>, line 1)
OK, let's see what is happening.
sage: from sage.misc.sageinspect import sage_getsource, sage_getargspec sage: from sage.algebras.letterplace.free_algebra_element_letterplace import poly_reduce sage: source = sage_getsource(poly_reduce.__call__) sage: print source def __call__(self, *args, ring=None, bint interruptible=True, attributes=None): ...
This actually looks fine, and rather easy to parse.
But in fact, _sage_getargspec_from_ast
is to blame, it seems:
sage: from sage.misc.sageinspect import _sage_getargspec_cython, _sage_getargspec_from_ast sage: _sage_getargspec_from_ast("def __call__(self, *args, ring, interruptible, attributes):") ------------------------------------------------------------ File "<unknown>", line 1 def __call__(self, *args, ring, interruptible, attributes): ^ SyntaxError: invalid syntax
Aha!!! That's very interesting!! Apparently Cython accepts this argument definition, but it is indeed a syntax error in Python!
sage: def dummy(self,*args,ring=None,interruptible=True,attributes=None): pass ------------------------------------------------------------ File "<ipython console>", line 1 def dummy(self,*args,ring=None,interruptible=True,attributes=None): pass ^ SyntaxError: invalid syntax sage: def dummy(self,ring=None,interruptible=True,attributes=None, *args): pass ....: sage:
So, what can one do? I could try to let _sage_getargspec_cython
determine the varargs and keywords name, and put them to the very end of the argument list. The list of arguments/argspec would not change, but it would be correct syntax in Python.
comment:16 follow-up: ↓ 17 Changed 7 years ago by
I would say it's a bug in Cython for allowing that syntax, but that doesn't help obviously...
I could try to let
_sage_getargspec_cython
determine the varargs and keywords name, and put them to the very end of the argument list. The list of arguments/argspec would not change, but it would be correct syntax in Python.
Sounds reasonable.
Alternatively, actually change
def __call__(self, *args, ring=None, bint interruptible=True, attributes=None)
to the Python-compatible
def __call__(self, ring=None, bint interruptible=True, attributes=None, *args)
comment:17 in reply to: ↑ 16 Changed 7 years ago by
- Status changed from needs_work to needs_review
Replying to jdemeyer:
I could try to let
_sage_getargspec_cython
determine the varargs and keywords name, and put them to the very end of the argument list. The list of arguments/argspec would not change, but it would be correct syntax in Python.Sounds reasonable.
I have updated my patch accordingly
Alternatively, actually change
def __call__(self, *args, ring=None, bint interruptible=True, attributes=None)to the Python-compatible
def __call__(self, ring=None, bint interruptible=True, attributes=None, *args)
But similar syntax occurs all over the place. I think that should be done on a separate ticket, if people think it is a problem.
Needs review, including a look at the outcome of the documentation as mentioned in comment:14.
comment:18 Changed 7 years ago by
PS: I have added more doctests, dealing with corner cases (no positional arguments, but only varargs or only keywords) and the good-in-Cython-but-bad-in-Python syntax.
comment:19 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to Fix variable name in pbori, accept pointer types in function definitions
Here is the outcome of building the reference guide from scratch, with the new patch:
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.problem_name: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_lp: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_mps: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.print_ranges: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.problem_name: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_lp: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_mps: Pointer types not allowed in def or cpdef functions /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/rings/polynomial/pbori.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.pbori.mod_mon_set: invalid syntax (<unknown>, line 1)
So, in most cases, there is complaint about a '*' being inside of the type definition, making it a pointer type. Let's have a look:
cpdef problem_name(self, char * name = NULL)
Arrgh. I thought no pointer was allowed, but char*
is an exception.
Since the only requirement is that sage_getargspec correctly parses correct syntax, a potential solution is to simply accept a "pointed" type definition, whether it is legal or not.
The last problem is different:
def mod_mon_set(BooleSet as, BooleSet vs):
as
is a reserved name in Python -- so, it is hard to believe that Cython accepts it as a variable name!
But in this case, I suggest to simply change the name in pbori.pyx, rather than trying to make _sage_getargspec_cython swallow it.
I'll do the two changes tomorrow.
Changed 7 years ago by
comment:20 Changed 7 years ago by
- Status changed from needs_work to needs_review
- Work issues Fix variable name in pbori, accept pointer types in function definitions deleted
It was a rather easy change, so, I updated the patch already. _sage_getargspec_cython is still entitled to complain about pointer types, but an exception from raising an exception is made (and doctested) for pointers to char. And I changed the critical variable name in pbori.pyx.
Needs review again (including docs).
comment:21 Changed 7 years ago by
For the record:
simon@linux-sqwp:~/SAGE/debug/sage-5.6.rc0/devel/sage-main> rm -r doc/output/ simon@linux-sqwp:~/SAGE/debug/sage-5.6.rc0/devel/sage-main> ../../sage -docbuild reference html sphinx-build -b html -d /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/output/doctrees/en/reference /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/output/html/en/reference Running Sphinx v1.1.2 loading pickled environment... not yet created loading intersphinx inventory from /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/common/python.inv... building [html]: targets for 1089 source files that are out of date updating environment: 1089 added, 0 changed, 0 removed reading sources... [100%] todolist looking for now-outdated files... none found pickling environment... done checking consistency... done preparing documents... done writing output... [100%] todolist writing additional files... genindex py-modindex search copying images... [100%] sage/combinat/../../media/combinat/complete-binary-trees-4.png copying static files... done dumping search index... done dumping object inventory... done build succeeded. Build finished. The built documents can be found in /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/output/html/en/reference
comment:22 Changed 7 years ago by
- Status changed from needs_review to positive_review
Documentation rebuilt cleanly for me as well. Looks good to me.
comment:23 Changed 7 years ago by
- Merged in set to sage-5.7.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
Here is our problem:
Perhaps
_sage_getargspec_cython
(which is called if it is suspected that the function is defined in Cython) should first check whether_sage_getargspec_from_ast
works.