Opened 9 years ago

Closed 8 years ago

#11298 closed defect (fixed)

Extend the capabilities of Sage's introspection

Reported by: SimonKing Owned by: jason
Priority: major Milestone: sage-4.7.2
Component: misc Keywords: edit sageinspect
Cc: jsrn, nthiery, saliola Merged in: sage-4.7.2.alpha1
Authors: Simon King Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #9976, #11287 Stopgaps:

Description (last modified by SimonKing)

The following used to fail:

sage: x??
Error getting source: could not find class definition
sage: P.<x,y> = QQ[]
sage: P??
Error getting source: could not find class definition
sage: I??
Error getting source: could not find class definition

Similarly, the edit command did not work in these case.

With my patch, all that works. Moreover, both sage.misc.edit_module and sage.misc.sageinspect are put into the reference manual.

Note that this may be related with #11287: sage.misc.sage_getfile and sage_getsourcelines could be a reliable tool to get information on how and from where to import a given object. Therefore Cc to the participants of that ticket.

Moreover, Cc to #9976. Reason:

Depends on #9976

Apply

Attachments (5)

trac11298_introspection_and_edit.patch (12.3 KB) - added by SimonKing 9 years ago.
Extend the capabilities of introspection and interactive source code edition
trac11298_singular_standard_options.patch (17.3 KB) - added by SimonKing 9 years ago.
Introspection for sage.misc.decorators.sage_wraps, applied for singular_standard_options. Put decorators into references.
trac11298_singular_standard_options.rebase4.7.1.a1.patch (17.3 KB) - added by burcin 9 years ago.
rebased Simon's patch to 4.7.1.alpha1
trac_11298-ref.patch (2.3 KB) - added by jhpalmieri 8 years ago.
referee patch; apply on top of others
trac11298_singular_standard_options.rebased.patch (17.3 KB) - added by SimonKing 8 years ago.
Introspection for sage.misc.decorators.sage_wraps, applied for singular_standard_options. Put decorators into references. Rebased against #11287

Download all attachments as: .zip

Change History (45)

comment:1 Changed 9 years ago by SimonKing

  • Status changed from new to needs_review

For the patchbot:

Depends on #9976

comment:2 Changed 9 years ago by SimonKing

Correcting a misspelling.

Depends on #9976

comment:3 Changed 9 years ago by SimonKing

Note that, with the patch, functools.partial objects can be reasonably edited as well:

edit(sage.combinat.partition_algebra.SetPartitionsAk,'vim')

opens vim at line 31 of sage/combinat/partition_algebra.py:

def create_set_partition_function(letter, k):
    """
    EXAMPLES::
    
        sage: from sage.combinat.partition_algebra import create_set_partition_function
        sage: create_set_partition_function('A', 3)
        Set partitions of {1, ..., 3, -1, ..., -3}
    """
    from sage.functions.all import floor
    if isinstance(k, (int, Integer)):
        if k > 0:
            return globals()['SetPartitions' + letter + 'k_k'](k)
    elif is_RealNumber(k):
        if k - math.floor(k) == 0.5:
            return globals()['SetPartitions' + letter + 'khalf_k'](floor(k))

    raise ValueError, "k must be an integer or an integer + 1/2"

and indeed that is the underlying definition of SetPartitionsAk; see line 53, which is

SetPartitionsAk = functools.partial(create_set_partition_function,"A")

comment:4 Changed 9 years ago by SimonKing

I found one more case where source code inspection failed: the (lib)singular_standard_options wrapper.

With the new patch, we have

sage: P.<x,y> = QQ[]
sage: I = P*[x,y]
sage: edit(I.interreduced_basis,'vim')
    ...
    @singular_standard_options
    @libsingular_standard_options
    def interreduced_basis(self):
        r"""
        If this ideal is spanned by `(f_1, ..., f_n)` this method
        returns `(g_1, ..., g_s)` such that:
        
        - `(f_1,...,f_n) = (g_1,...,g_s)`
        
        - `LT(g_i) != LT(g_j)` for all `i != j`
   ...

which would previously just show the code of the wrapper, not of the wrapped method.

I wonder why the patchbot complains. In particular, why does the patchbot mention my patch from trac ticket #11115 - it shouldn't be a dependency, or should it? Let's see.

For now:

Depends on #9976

comment:5 Changed 9 years ago by SimonKing

I just verified: In my patch queue, the patches from #11115 come after trac11298_introspection_and_edit.patch

In other words, the behaviour of the patchbot seems very strange to me.

comment:6 Changed 9 years ago by SimonKing

I still think that the patchbot goofed it, but I did as well: The old version of the second patch would really be on top of #11115 (but not the first). I was just updating it, so that we have:

Depends on #9976

and nothing else. Could be that I need to fix some tests, though...

comment:7 Changed 9 years ago by SimonKing

Adding "r" to one doc string (for making the documentation nicely formatted) and correcting one doc test.

The tests in sage.misc and in sage.rings.polynomial all pass. Next, I'll run long tests.

Depends on #9976

comment:8 Changed 9 years ago by SimonKing

  • Status changed from needs_review to needs_work

I'll try to trigger the patchbot by changing to "needs work"...

comment:9 Changed 9 years ago by SimonKing

... and returning to needs review...

Depends on #9976

comment:10 Changed 9 years ago by SimonKing

  • Status changed from needs_work to needs_review

Changed 9 years ago by SimonKing

Extend the capabilities of introspection and interactive source code edition

comment:11 Changed 9 years ago by SimonKing

OK, that did not help. Hence, I submitted the patches again. I wish the red blob would finally vanish!

Depends on #9976

comment:12 Changed 9 years ago by SimonKing

  • Dependencies set to #9976

Ooops, I forgot that since recently there is a form field in which to state dependencies. Let's try it!

comment:13 Changed 9 years ago by SimonKing

FWIW, long tests pass for me, and I believe that the patchbot has a serious flaw if it can not even deal with a single dependency.

comment:14 Changed 9 years ago by SimonKing

Now I understand what the patchbot is doing (resp. not doing): It believes that #9976 does not need to be applied, since it is merged. But it does not understand that #9976 is merged in 4.7.1.alpha0, not in 4.7.rc1 (which is what it is starting with).

comment:15 Changed 9 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Fix crash at docbuild

I noticed that building the documentation crashes when the second patch is applied. It is as follows:

Running Sphinx v1.0.4
loading pickled environment... done
building [html]: targets for 39 source files that are out of date
updating environment: 0 added, 39 changed, 0 removed
reading sources... [ 33%] sage/rings/polynomial/multi_polynomial_ideal_libsingular                           
Exception occurred:
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/config.py", line 207, in __getattr__
    raise AttributeError('No such config value: %s' % name)
AttributeError: No such config value: autodoc_default_flags
The full traceback has been saved in /tmp/sphinx-err-1tNBmd.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
Either send bugs to the mailing list at <http://groups.google.com/group/sphinx-dev/>,
or report them in the tracker at <http://bitbucket.org/birkenfeld/sphinx/issues/>. Thanks!
Build finished.  The built documents can be found in /mnt/local/king/SAGE/sage-4.7.rc2/devel/sage/doc/output/html/en/reference

and the traceback is

# Sphinx version: 1.0.4
# Docutils version: 0.5 release
# Jinja2 version: 2.5.5
Traceback (most recent call last):
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/cmdline.py", line 173, in main
    app.build(force_all, filenames)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/application.py", line 207, in build
    self.builder.build_update()
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/builders/__init__.py", line 198, in build_update
    'out of date' % len(to_build))
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/builders/__init__.py", line 218, in build
    purple, length):
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/builders/__init__.py", line 120, in status_iterator
    for item in iterable:
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/environment.py", line 518, in update_generator
    self.read_doc(docname, app=app)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/environment.py", line 658, in read_doc
    pub.publish()
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/core.py", line 204, in publish
    self.settings)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/readers/__init__.py", line 69, in read
    self.parse()
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/readers/__init__.py", line 75, in parse
    self.parser.parse(self.input, document)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/__init__.py", line 157, in parse
    self.statemachine.run(inputlines, document, inliner=self.inliner)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 170, in run
    input_source=document['source'])
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/statemachine.py", line 232, in run
    context, state, transitions)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/statemachine.py", line 420, in check_line
    return method(match, context, next_state)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 2658, in underline
    self.section(title, source, style, lineno - 1, messages)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 308, in section
    self.new_subsection(title, lineno, messages)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 376, in new_subsection
    node=section_node, match_titles=1)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 266, in nested_parse
    node=node, match_titles=match_titles)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 195, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/statemachine.py", line 232, in run
    context, state, transitions)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/statemachine.py", line 420, in check_line
    return method(match, context, next_state)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 2241, in explicit_markup
    self.explicit_list(blank_finish)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 2269, in explicit_list
    match_titles=self.state_machine.match_titles)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 300, in nested_list_parse
    node=node, match_titles=match_titles)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 195, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/statemachine.py", line 232, in run
    context, state, transitions)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/statemachine.py", line 420, in check_line
    return method(match, context, next_state)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 2542, in explicit_markup
    nodelist, blank_finish = self.explicit_construct(match)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 2251, in explicit_construct
    return method(self, expmatch)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 1994, in directive
    directive_class, match, type_name, option_presets)
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/docutils/parsers/rst/states.py", line 2043, in run_directive
    result = directive_instance.run()
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/ext/autodoc.py", line 1154, in run
    if flag in self.env.config.autodoc_default_flags and \
  File "/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/Sphinx-1.0.4-py2.6.egg/sphinx/config.py", line 207, in __getattr__
    raise AttributeError('No such config value: %s' % name)
AttributeError: No such config value: autodoc_default_flags

I have no idea how this can be debugged.

comment:16 Changed 9 years ago by SimonKing

Perhaps it would be worth while to follow a more conceptual approach.

Currently, singular_standard_options and magma_standard_options applied to a method func simply return a function wrapper, that does not know about func (except its doc string, but I think that's a bad idea as well since copying that string costs time). So, introspection does not work. I tried to provide wrapper with the tools needed to make it work, but apparently it was not so easy.

But there is the sage_wraps factory, and I think I should try to use it!

Changed 9 years ago by SimonKing

Introspection for sage.misc.decorators.sage_wraps, applied for singular_standard_options. Put decorators into references.

comment:17 Changed 9 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues Fix crash at docbuild deleted

The second patch has now been updated.

The problem seems solved: I touched all files in sage/rings/polynomial, did sage -b and then built the documentation without problem.

Moreover, I fixed some syntax error in the documentation, and I've put sage.misc.decorators into the reference manual.

So, it is needing review again!

Changed 9 years ago by burcin

rebased Simon's patch to 4.7.1.alpha1

comment:18 Changed 9 years ago by burcin

  • Description modified (diff)

comment:19 Changed 9 years ago by SimonKing

Thank you, Burcin!

For the patchbot, that seems to ignore the ticket description:

Apply trac11298_introspection_and_edit.patch, trac11298_singular_standard_options.rebase4.7.1.a1.patch

comment:20 follow-ups: Changed 8 years ago by jhpalmieri

This looks pretty good to me. I have one question: in sage_getsourcelines, lines 1408-1411, you have the code

            except IOError:
                if obj.__class__ != type:
                    return sage_getsourcelines(obj.__class__)
                raise

What is type here? Can you give me an example which will reach this code?

comment:21 Changed 8 years ago by jhpalmieri

(Is it just the built-in Python type function?)

comment:22 in reply to: ↑ 20 Changed 8 years ago by SimonKing

Replying to jhpalmieri:

This looks pretty good to me. I have one question: in sage_getsourcelines, lines 1408-1411, you have the code ...

What is type here? Can you give me an example which will reach this code?

By type, I mean the type type, such as here:

sage: 1.__class__
<type 'sage.rings.integer.Integer'>
sage: 1.__class__.__class__
<type 'type'>

The idea of the code lines in question is as follows:

Let obj be the object whose source lines we want to determine. It is not necessarily true that the correct source lines for obj coincide with the source lines of its class (notorious counter example: decorated methods). Therefore, we need to try harder.

  • First, it is attempted to get the position of the source code from what inspect.getdoc returns (lines 1400-1401)
  • Second (if "first" fails), the same is attempted with what _sage_getdoc_unformatted returns (lines 1403-1404). For a reason that I don't understand, one can not replace the first step by the second step: The comment that I inserted states that a mysterious import error would occur at startup.
  • Third (if "second" fails as well), it is tested whether the inspect module can provide the source lines of the object obj (line 1407).
  • Last resort (if "third" fails), we replace obj by obj.__class__. That is not desired for decorated methods, but these are supposed to provide the source lines in a different way, so that one of the first three methods should succeed. I think as a last resort it is ok... (line 1410).

There is one obvious problem with the "last resort". If the source lines of obj.__class__ can not be found (e.g., if obj.__class__ is defined in an spkg, not in the Sage library) then as a last resort one would try to get obj.__class__.__class__, but that is the type type (see example above), which is defined in the Python spkg and can thus not be found. Infinite recursion.

In order to avoid the infinite recursion, an error is raised if obj.__class__ is the type type, which is, in particular, the case if obj is not a class instance but a class.

I hope that answers your question...

comment:23 in reply to: ↑ 20 Changed 8 years ago by SimonKing

Replying to jhpalmieri:

What is type here? Can you give me an example which will reach this code?

A concrete example (using the meataxe wrapper from my cohomology spkg):

sage: from sage.misc.sageinspect import sage_getsourcelines
sage: from pGroupCohomology.mtx import MTX
sage: M = MTX(5,[[1,2],[3,4]])
sage: sage_getsourcelines(M)
---------------------------------------------------------------------------
IOError                                   Traceback (most recent call last)

/home/king/<ipython console> in <module>()

/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/sage/misc/sageinspect.pyc in sage_getsourcelines(obj, is_binary)
   1407         if pos is None:
   1408             try:
-> 1409                 return inspect.getsourcelines(obj)
   1410             except IOError:
   1411                 if obj.__class__ != type:

/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python/inspect.pyc in getsourcelines(object)
    676     original source file the first line of code was found.  An IOError is
    677     raised if the source code cannot be retrieved."""
--> 678     lines, lnum = findsource(object)
    679
    680     if ismodule(object): return lines, 0

/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/IPython/ultraTB.pyc in findsource(object)
    175             return lines, candidates[0][1]
    176         else:
--> 177             raise IOError('could not find class definition')
    178
    179     if ismethod(object):

IOError: could not find class definition

Since M can not provide its source lines, the source lines of M.__class__ (i.e., of MTX) are sought.

Since the source lines of MTX can not be found (with the first three methods), the inspect module raises a IOError. It is caught, but reraised, since MTX.__class__ is the type type and hence an infinite recursion must be prevented.

comment:24 follow-up: Changed 8 years ago by jhpalmieri

  • Reviewers set to John Palmieri

Okay, I'm happy with this. I'm attaching a referee's patch with a few small changes. If you're happy with my changes, please switch the ticket to "positive review".

This makes a very nice companion to #9976.

Changed 8 years ago by jhpalmieri

referee patch; apply on top of others

comment:25 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

(The only aspect of the referee's patch worth mentioning is the removal of the import of sage_getsource. This is already done at the top-level of that file, so it doesn't need to be done again.)

comment:26 in reply to: ↑ 24 Changed 8 years ago by SimonKing

  • Status changed from needs_review to positive_review

Replying to jhpalmieri:

Okay, I'm happy with this. I'm attaching a referee's patch with a few small changes. If you're happy with my changes, please switch the ticket to "positive review".

I am happy with the changes. Thus I change it into a positive review. Thank you very much!

comment:27 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:28 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase to #11287

This needs to be rebased to #11287.

comment:29 follow-up: Changed 8 years ago by jhpalmieri

After applying the patch at #11287, these still apply for me, although one does have fuzz. Is the fuzz an issue or are patches actually being rejected?

comment:30 Changed 8 years ago by SimonKing

  • Status changed from needs_work to needs_info

<Bump>

Jeroen, could you please elaborate why you think that it should be rebased?

comment:31 Changed 8 years ago by jhpalmieri

(If all else fails, we can wait until 4.7.2.alpha0 and rebase it with respect to that.)

comment:32 in reply to: ↑ 29 ; follow-up: Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

After applying the patch at #11287, these still apply for me, although one does have fuzz. Is the fuzz an issue or are patches actually being rejected?

My personal opinion on applying patches with fuzz is that I allow patches with fuzz 1 but not with fuzz 2. I have seen at least two cases where a "fuzz 2" patch was actually wrongly merged (I don't remember which tickets).

comment:33 in reply to: ↑ 32 Changed 8 years ago by SimonKing

Replying to jdemeyer:

My personal opinion on applying patches with fuzz is that I allow patches with fuzz 1 but not with fuzz 2. I have seen at least two cases where a "fuzz 2" patch was actually wrongly merged (I don't remember which tickets).

OK. Then I'll take care of it.

comment:34 Changed 8 years ago by SimonKing

By the way: When I tried to import the patch from #11287 on top of sage-4.7.rc2, application to doc/en/reference/mist.rst failed. But if I understood correctly, the problem with the patch here is somewhere else, right? And moreover, I should probably get a more recent Sage version...

comment:35 follow-up: Changed 8 years ago by SimonKing

That's odd!

I applied the patches from #11251, then #11287. Then I had fuzz 2 for one of the patches from #9976 (but they are already merged in 4.7.1.alpha0), and then the patches from here applied cleanly and without fuzz.

So, it seems that I should get the latest rc or alpha version of sage, and try again.

comment:36 in reply to: ↑ 35 Changed 8 years ago by jdemeyer

Replying to SimonKing: Yes, sage-4.7.rc2 is in fact quite old, so please try sage-4.7.1.rc1 + #11287 (or wait until sage-4.7.2.alpha0 is released and rebase to that).

Changed 8 years ago by SimonKing

Introspection for sage.misc.decorators.sage_wraps, applied for singular_standard_options. Put decorators into references. Rebased against #11287

comment:37 Changed 8 years ago by SimonKing

  • Dependencies changed from #9976 to #9976, #11287
  • Description modified (diff)
  • Status changed from needs_info to needs_review

I've built sage-4.7.1.rc1 and rebased the patch.

Apply trac11298_introspection_and_edit.patch attachment:trac11298_singular_standard_options.rebased.patch attachment:trac_11298-ref.patch

comment:38 Changed 8 years ago by SimonKing

  • Status changed from needs_review to positive_review

The change of the patch concerns doc/en/misc.rst, and the reason for the change was the fact that the context of the inserted line has changed. In particular, the change in the patch has not changed code or doctests. Therefore I believe that it should be reverted to a positive review.

Apply trac11298_introspection_and_edit.patch trac11298_singular_standard_options.rebased.patch trac_11298-ref.patch

comment:39 Changed 8 years ago by jhpalmieri

I believe that it should be reverted to a positive review.

I agree. If it's a simple rebase, no new review should be needed. Thanks for taking care of this.

comment:40 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
  • Work issues rebase to #11287 deleted
Note: See TracTickets for help on using tickets.