Opened 10 years ago

Closed 10 years ago

#8325 closed defect (fixed)

Sphinx warning: 'Could not parse cython argspec'

Reported by: mpatel Owned by:
Priority: minor Milestone: sage-4.3.4
Component: documentation Keywords:
Cc: jhpalmieri Merged in: sage-4.3.4.alpha1
Authors: Mitesh Patel Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Sphinx warnings from building the HTML reference manual include:

matrix/matrix_integer_dense.rst:6: (WARNING/2) error while formatting signature for sage.matrix.matrix_integer_dense.Matrix_integer_dense.LLL: Could not parse cython argspec
plot/plot3d/base.rst:6: (WARNING/2) error while formatting signature for sage.plot.plot3d.base.Graphics3d.export_jmol: Could not parse cython argspec

Related: #8244.

Attachments (5)

trac_8325-cython_argspec.patch (2.1 KB) - added by mpatel 10 years ago.
Alternate way to get Cython argspec. sage repo.
trac_8325-cython_argspec.2.patch (6.1 KB) - added by mpatel 10 years ago.
AST version. sage repo.
trac_8325-cython_argspec.3.patch (10.2 KB) - added by mpatel 10 years ago.
Another clause. More doctests. Apply only this patch. sage repo.
sagenb_8325-cython_argspec.patch (10.2 KB) - added by mpatel 10 years ago.
For SageNB's sageinspect.py. sagenb repo.
sagenb_8325-cython_argspec.2.patch (10.3 KB) - added by mpatel 10 years ago.
Fix doctest imports. Replaces previous sagenb patch.

Download all attachments as: .zip

Change History (21)

Changed 10 years ago by mpatel

Alternate way to get Cython argspec. sage repo.

comment:1 Changed 10 years ago by mpatel

  • Cc jhpalmieri added
  • Status changed from new to needs_review

The patch should work for the cases in the description.

comment:2 Changed 10 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Should patches like this also come with a corresponding one to sagenb.misc.sageinspect, as in #8324?

Looks good, no warnings when building the reference manual for these two files, and the relevant pages look good. Nice solution.

comment:3 follow-up: Changed 10 years ago by jhpalmieri

(Is it dangerous to use "exec" here? It looks okay to me, but I feel as though I should always be suspicious of using it.)

comment:4 in reply to: ↑ 3 Changed 10 years ago by mpatel

Replying to jhpalmieri:

(Is it dangerous to use "exec" here? It looks okay to me, but I feel as though I should always be suspicious of using it.)

I think you're right. The exec'd code could have bad side effects. I'm about to attach a patch that uses this AST code, instead.

comment:5 Changed 10 years ago by mpatel

  • Status changed from positive_review to needs_work

Changed 10 years ago by mpatel

AST version. sage repo.

comment:6 Changed 10 years ago by mpatel

  • Status changed from needs_work to needs_review

I've attached the AST version, which seems to work for me, although I have no formal training in computer science. If the patch looks good, I'll add a sagenb repository patch.

comment:7 Changed 10 years ago by jhpalmieri

On one hand, this seems to fix the two particular doctests in question. On the other, it's not perfect. I can see two problems, one of which I know how to fix:

  • if the source has the (unlikely) form def f({(1,2,3): True}): ..., then this version (and all previous versions) will think that the arg spec ends after (1,2,3):. The function _sage_getargspec_from_ast can actually handle this kind of thing, though, so I think we should pass the entire source code to it, rather than truncate at the first ):. That is, delete line 470 and change line 471 from
    proxy = 'def dummy' + source[beg:end] + '\n    return' 
    
    to
    proxy = 'def dummy' + source[beg:] + '\n    return' 
    
  • if the docstring has type information, it can't handle it. I don't know what to do about this, or if it's worth it.

Should we fix the first problem and then go ahead with this? I also notice that the methods for SageArgSpecVisitor don't have doctests. Is that possible for these sorts of things? I know nothing about ast.

Changed 10 years ago by mpatel

Another clause. More doctests. Apply only this patch. sage repo.

comment:8 Changed 10 years ago by mpatel

V3:

  • Implements your idea to pass the entire source code. I've included the previous version as a fallback, since LLL has a Cython-only construct:
    R = <Matrix_integer_dense>self.new_matrix(entries=map(ZZ,A.list()))
    
  • Has extra, more direct doctests of SageArgSpecVisitor's methods.

I don't mind leaving the second problem for another day.

comment:9 Changed 10 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Looks good. All doctests pass, fixes the two problems.

Changed 10 years ago by mpatel

For SageNB's sageinspect.py. sagenb repo.

comment:10 Changed 10 years ago by mpatel

  • Status changed from positive_review to needs_work

I've attached a version for SageNB.

comment:11 Changed 10 years ago by mpatel

  • Status changed from needs_work to needs_review

comment:12 Changed 10 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

Unless I'm misunderstanding, the sagenb patch needs work: all of the doctests in the patch import from sage.misc.sageinspect rather than from sagenb....

Changed 10 years ago by mpatel

Fix doctest imports. Replaces previous sagenb patch.

comment:13 Changed 10 years ago by mpatel

  • Status changed from needs_work to needs_review

Oops. Sorry about that. V2 is up.

comment:14 Changed 10 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

comment:15 Changed 10 years ago by mhansen

  • Merged in set to sage-4.3.4.alpha1
  • Owner changed from mvngu to (none)

I've merged the Sage library patch. When this is merged in the notebook, we can close this ticket.

comment:16 Changed 10 years ago by mpatel

  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged sagenb patch V2 in sagenb-0.7.5.3, which will be at #8435.

Note: See TracTickets for help on using tickets.