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)
Change History (21)
comment:1 Changed 10 years ago by
- 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
- 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: ↓ 4 Changed 10 years ago by
(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
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
- Status changed from positive_review to needs_work
comment:6 Changed 10 years ago by
- 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
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 fromproxy = 'def dummy' + source[beg:end] + '\n return'
toproxy = '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.
comment:8 Changed 10 years ago by
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
- Status changed from needs_review to positive_review
Looks good. All doctests pass, fixes the two problems.
comment:10 Changed 10 years ago by
- Status changed from positive_review to needs_work
I've attached a version for SageNB.
comment:11 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:12 Changed 10 years ago by
- 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...
.
comment:13 Changed 10 years ago by
- Status changed from needs_work to needs_review
Oops. Sorry about that. V2 is up.
comment:14 Changed 10 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 10 years ago by
- 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
- 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.