Opened 8 years ago

Closed 7 years ago

#11768 closed enhancement (fixed)

Get source code for parent/element classes of categories

Reported by: SimonKing Owned by: jason
Priority: major Milestone: sage-5.6
Component: misc Keywords: sources dynamic class
Cc: nthiery, hivert Merged in: sage-5.6.beta1
Authors: Simon King Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by SimonKing)

Currently (sage-4.7.2.alpha2) one can not interactively get the source code of the parent or element classes of a category:

sage: C.parent_class??
Type:           DynamicMetaclass
Base Class:     <class 'sage.structure.dynamic_class.DynamicMetaclass'>
String Form:    <class 'sage.categories.rings.Rings.parent_class'>
Namespace:      Interactive
File:           /mnt/local/king/SAGE/sage-4.7.2.alpha1/local/lib/python2.6/site-packages/sage/categories/rings.py
Source:
class DynamicMetaclass(type):
    """
    A metaclass implementing an appropriate reduce-by-construction method
    """

    def __reduce__(self):
        """
        See sage.structure.dynamic_class.dynamic_class? for non trivial tests.

        TESTS::

...

As one can see, it gets the source file right, but can not find the class definition in it.

I tried to track it down.

  • For a dynamic metaclass, we want to obtain the documentation and source of the base class (resp. the class that is passed too the dynamic metaclass by the doccls argument).
  • The module is correctly provided, and thus Python's inspect module is able to find the source file.
  • If it is a class then Python's inspect module tries to find something like "class "+cls.__name__+":" in the sources (they use a more sophisticated regular expression).
  • The base class is Rings().ParentMethods. Unfortunately, it is defined by class ParentMethods, but the name is differently:
    sage: Rings().ParentMethods.__name__
    'Rings.ParentMethods'
    

I do not suggest to change the name: If the name is just 'ParentMethods' then Python's inspect module would always find the first occurence of that name, which is bad if one module defines several categories.

Instead, I suggest to create a new function in sage.misc.sageinspect that recursively finds the sources for classes whose sources can't be found by other methods and whose name contains a dot:

The function that I plan to write does not start with the whole source file of sage.categories.rings, but it starts with the sources of sage.categories.rings.Rings and then searches class ParentMethods (with the regular expression from the inspect module) only in that small part of the full source file.

I think "getting sources" belongs to the component "misc", even though the problem is relevant for not getting confused by the category framework. Thus, cc to Nicolas...

Apply

Attachments (1)

trac11768_source_of_dynamic_class.patch (11.7 KB) - added by SimonKing 7 years ago.
Get source lines of dynamic classes (e.g., parent_class of a category). Combined patch

Download all attachments as: .zip

Change History (42)

comment:1 Changed 8 years ago by SimonKing

The patch works exactly as it is announced in the ticket description. One can now (on top of sage-4.7.2.alpha2) do:

sage: C = Rings()
sage: from sage.misc.sageinspect import sage_getsource
sage: print sage_getsource(C.parent_class)  #indirect doctest
class ParentMethods:
    def is_field(self):
        raise NotImplementedError
<BLANKLINE>
    def bracket(self, x, y):
...

Unfortunately,

sage: P = JackPolynomialsP(QQ); P.rename("JackP")
sage: P.element_class??

does not work yet. So, the search for definitions should also include the __base__ attribute of a class.

comment:2 Changed 8 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

It seems to me that things are fine now.

In particular, we have

sage: C = Rings()
sage: C.parent_class??
Type:           DynamicMetaclass
Base Class:     <class 'sage.structure.dynamic_class.DynamicMetaclass'>
String Form:    <class 'sage.categories.rings.Rings.parent_class'>
Namespace:      Interactive
File:           /mnt/local/king/SAGE/sage-4.7.2.alpha1/local/lib/python2.6/site-packages/sage/categories/rings.py
Source:
    class ParentMethods:
        def is_field(self):
            raise NotImplementedError

        def bracket(self, x, y):
...
sage: P = JackPolynomialsP(QQ)
sage: P.element_class??
Type:           DynamicMetaclass
Base Class:     <class 'sage.structure.dynamic_class.DynamicMetaclass'>
String Form:    <class 'sage.combinat.sf.jack.JackPolynomials_p_with_category.element_class'>
Namespace:      Interactive
File:           /mnt/local/king/SAGE/sage-4.7.2.alpha1/local/lib/python2.6/site-packages/sage/combinat/sf/jack.py
Definition:     P.element_class(self, x, include=None, exclude=None)
Source:
class SymmetricFunctionAlgebra_generic_Element(CombinatorialFreeModule.Element):
    def __repr__(self):
        """
        EXAMPLES::

            sage: m = SFAMonomial(QQ)
...
sage: P.<x,y> = QQ[]
sage: I = P*[x,y]
sage: I??
Type:           MPolynomialIdeal
Base Class:     <class 'sage.rings.polynomial.multi_polynomial_ideal.MPolynomialIdeal'>
String Form:    Ideal (x, y) of Multivariate Polynomial Ring in x, y over Rational Field
Namespace:      Interactive
File:           /mnt/local/king/SAGE/sage-4.7.2.alpha1/local/lib/python2.6/site-packages/sage/rings/polynomial/multi_polynomial_ideal.py
Source:
class MPolynomialIdeal( MPolynomialIdeal_singular_repr, \
                        MPolynomialIdeal_macaulay2_repr, \
                        MPolynomialIdeal_magma_repr, \
                        Ideal_generic ):
    def __init__(self, ring, gens, coerce=True):
        r"""
        Create an ideal in a multivariate polynomial ring.
...

All three examples are doctested (via sage_getsourcelines). So, ready for review!

comment:3 Changed 8 years ago by SimonKing

  • Milestone changed from sage-5.0 to sage-4.7.2

Perhaps it would work to get it in sage-4.7.2, not only sage-5.0?

comment:4 Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work

I just found one thing that doesn't work, yet.

With

sage: C = Rings()
sage: HC = C.hom_category()

neither HC?? nor edit(HC,'vim') do what one would wish.

comment:5 Changed 8 years ago by SimonKing

  • Status changed from needs_work to needs_review

Introspection of Rings().hom_category() works and is doctested with the updated patch. Back to "needs review".

comment:6 Changed 8 years ago by nthiery

Hi Simon!

Definitely +1 on the feature! Thanks!

I had a quick look at the implementation, and it sounds reasonable, though I do not yet have an opinion yet. Any sageinspect expert around?

comment:7 Changed 8 years ago by hivert

  • Cc hivert added

I'm also +1 on the feature. I just want point out the following trap. With double class nesting the name mangling is currently incorrect (see #9107). Unfortunately, I won't have time to work on it (ping Nicolas ;-). There is a patch in combinat's queue ( trac_9107-nested_class_bug-fh.patch) which experiment on what is happening.

comment:8 Changed 8 years ago by SimonKing

Hi Florent,

Thank you for pointing to the other ticket! #9107 seems quite mysterious to me. Clearly, in the example exposed there, my way to get to the sources would fail. Of course, the current way would fail even more...

comment:9 follow-up: Changed 8 years ago by vbraun

Fails to apply to vanilla sage-4.7.2.alpha2. Dependencies?

comment:10 in reply to: ↑ 9 Changed 8 years ago by SimonKing

Replying to vbraun:

Fails to apply to vanilla sage-4.7.2.alpha2. Dependencies?

Actually I don't know (I don't remember that I had anything else added when I worked on the patch). But it should apply to sage-4.7.2.alpha3-prerelease.

comment:11 Changed 8 years ago by SimonKing

Do you really start with vanilla sage-4.7.2.alpha2? I just tested, and I have (sorry for the German):

king@mpc622:/mnt/local/king/SAGE/debug/sage-4.7.2.alpha2/devel/sage$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/11768/trac11768_source_of_dynamic_class.patch
Füge trac11768_source_of_dynamic_class.patch zur Seriendatei hinzu
king@mpc622:/mnt/local/king/SAGE/debug/sage-4.7.2.alpha2/devel/sage$ hg qpush
Wende trac11768_source_of_dynamic_class.patch an
jetzt bei: trac11768_source_of_dynamic_class.patch
king@mpc622:/mnt/local/king/SAGE/debug/sage-4.7.2.alpha2/devel/sage$ hg qapplied 
trac11768_source_of_dynamic_class.patch

So, it does apply to vanilla sage-4.7.2.alpha2 without fuzz.

comment:12 Changed 8 years ago by vbraun

Sorry, you are right. It does apply. Must have forgotten to pop all patches %-)

comment:13 Changed 8 years ago by SimonKing

What is the patchbot complaining about? It states that it fails to apply, but I find it is fine with 4.7.2-alpha3-prerelease.

comment:14 Changed 8 years ago by SimonKing

Any reviewer for that ticket? I think introspection is a nice thing to have in more generality...

comment:15 Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Make it independent of #11900

It turns out that a combination of this ticket with #11900 yields a problem. I'll try to rewrite the patch here, so that it doesn't matter whether #11900 is applied or not.

comment:16 Changed 8 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues Make it independent of #11900 deleted

Done! Using the new patch, the doc tests of sage/misc/sageinspect.py pass regardless whether #11900 is applied or not.

comment:17 Changed 8 years ago by SimonKing

With the patch, one still has

sage: C = Algebras(ZZ).TensorProducts()
sage: C.ParentMethods??
Error getting source: could not get source code
Type:           classobj
String Form:    sage.categories.algebras.TensorProducts.ParentMethods
Namespace:      Interactive
File:           /mnt/local/king/SAGE/rebase/sage-4.8.alpha0/local/lib/python2.6/site-packages/sage/categories/algebras.py

However, if C is the category of Z-algebras, then C.ParentMethods?? works nicely. So, the patch isn't perfect, but it is a progress. Review, anyone?

comment:18 Changed 8 years ago by SimonKing

If #12876 is applied then one of the tests introduced here gives an unexpected result: We test against a particular code snipped, that changes with #12876.

Hence, I'll rewrite the test so that it expects only those parts of the code that do not change.

comment:19 Changed 8 years ago by SimonKing

  • Description modified (diff)

With the second patch, the doctest becomes independent of whether or not #12876 is applied.

Apply trac11768_source_of_dynamic_class.patch trac11768_docfix.patch

comment:20 Changed 8 years ago by SimonKing

Cool! I just notice:

sage: C = Algebras(ZZ).TensorProducts()
sage: C.ParentMethods??
Type:		classobj
String Form:	sage.categories.algebras.Algebras.TensorProducts.ParentMethods
Namespace:	Interactive
Loaded File:	/mnt/local/king/SAGE/stable/sage-5.0.beta13/local/lib/python2.7/site-packages/sage/categories/algebras.py
Source File:	/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/algebras.py
Source:
        class ParentMethods:
            #def coproduct(self):
            #    tensor products of morphisms are not yet implemented
            #    return tensor(module.coproduct for module in self.modules)
            pass

I don't know who solved that, but (as I stated above) that used to fail!

comment:21 follow-up: Changed 8 years ago by nthiery

By the way: feel free to steal this little fix which I did take the time not push through ...:

http://combinat.sagemath.org/patches/file/tip/sage_inspect_for_classes-nt.patch

comment:22 in reply to: ↑ 21 ; follow-up: Changed 8 years ago by SimonKing

Replying to nthiery:

By the way: feel free to steal this little fix which I did take the time not push through ...:

??

According to the comments in the patch, it would not solve the problem addressed here:

Now sage_getsourcelines works similarly for old and new style classes:

sage: sage_getsourcelines(Sets.ParentMethods)
sage: sage_getsourcelines(Sets.Algebras)

Still both fail, because inspect.getsourcelines (in fact
inspect.findsource) fails on nested classes which have been renamed
with NestedClassMetaclass:
...

The point is that both do work with my patch:

sage: from sage.misc.sageinspect import sage_getsourcelines
sage: for x in sage_getsourcelines(Sets.Algebras)[0]:             
....:     print x,
....:     
    class Algebras(AlgebrasCategory):

        def extra_super_categories(self):
            """
            EXAMPLES::

                sage: Sets().Algebras(QQ).extra_super_categories()
                [Category of modules with basis over Rational Field]

                sage: Sets().Algebras(QQ).super_categories()
                [Category of modules with basis over Rational Field]

                sage: Sets().example().algebra(ZZ).categories()
                [Category of set algebras over Integer Ring,
                 Category of modules with basis over Integer Ring,
                 ...
                 Category of objects]

            """
            from sage.categories.modules_with_basis import ModulesWithBasis
            return [ModulesWithBasis(self.base_ring())]

comment:23 in reply to: ↑ 22 Changed 8 years ago by nthiery

Replying to SimonKing:

Replying to nthiery:

By the way: feel free to steal this little fix which I did take the time not push through ...:

According to the comments in the patch, it would not solve the problem addressed here:

Now sage_getsourcelines works similarly for old and new style classes:

sage: sage_getsourcelines(Sets.ParentMethods)
sage: sage_getsourcelines(Sets.Algebras)

Still both fail, because inspect.getsourcelines (in fact
inspect.findsource) fails on nested classes which have been renamed
with NestedClassMetaclass:
...

Oh, sorry, I was unclear. I did not mean that this patch would fix this ticket. Just that it fixed another little issue in a related spot in the code. If your patch already takes care of it, so much the better!

Cheers,

Nicolas

comment:24 Changed 7 years ago by SimonKing

Any review for better access to source code in the category framework?

comment:25 follow-up: Changed 7 years ago by vbraun

Looks good. You might want to pick classes for doctests that are not moving targets in the combinat queue, though ;-) How about sage/structure/sage_object.py or something else thats buried deep down and unlikely to change.

comment:26 in reply to: ↑ 25 Changed 7 years ago by SimonKing

Replying to vbraun:

Looks good. You might want to pick classes for doctests that are not moving targets in the combinat queue, though ;-) How about sage/structure/sage_object.py or something else thats buried deep down and unlikely to change.

Good idea. I can not remember why I chose JackPolynomials to create examples. I'll see if SageObject.element_class shows the same problems with introspection than JackPolynomials.element_class.

comment:27 Changed 7 years ago by SimonKing

I think what I need is:

  • A parent class whose metaclass is a sub-class of NestedClassMetaclass (such as UniqueRepresentation)
  • ... so that the Element attribute is given as a nested class.

These two properties hold for the JackPolynomials example.

What I could do is to create a minimal example in the doc tests. Unfortunately, I need a source file to make it work -- hence, such a doc test needs to use the cython(...) function and hence needs a compiler.

I did create a couple of examples of that type in other patches that got merged, but some people said it is bad practice to have examples that rely on a compiler.

So, what do you think? Shall I use the compiler, or perhaps create a new class in the Sage library just for the purpose of testing?

comment:28 Changed 7 years ago by SimonKing

I don't know yet whether my patch would fix the following example:

sage: class MyParent(UniqueRepresentation, Parent):
....:    class Element:
....:        pass
....:     
sage: P = MyParent(ZZ)
sage: P.element_class??
Type:           DynamicMetaclass
Base Class:     <class 'sage.structure.dynamic_class.DynamicMetaclass'>
String Form:    <class '__main__.MyParent.element_class'>
Namespace:      Interactive
Loaded File:    /home/simon/SAGE/debug/sage-5.5.rc0/local/lib/python2.7/site-packages/IPython/FakeModule.py
Source File:    /home/simon/SAGE/debug/sage-5.5.rc0/devel/sage/IPython/FakeModule.py
Source:
class DynamicMetaclass(type):
...

If I understand correctly, my patch would fix a corresponding example in Cython (because it then has a source file to inspect). If my patch is able to fix the example above then I'll use it.

comment:29 Changed 7 years ago by SimonKing

To my surprise, the patch does not solve the introspection problem mentioned in my previous post, even if Cython code is used.

I think to make it work interactively should be moved on a different ticket. So, for now, I suggest to create a test class in sage.misc.sageinspect.

comment:30 Changed 7 years ago by SimonKing

  • Description modified (diff)

I have folded the two patches into one, and I replaced some occurences of trac ticket #... by the appropriate :trac: directive.

And of course I removed the JackPolynomial test. Instead, the sources of a dummy class are inspected, created only for testing.

It turned out to be impossible to put the dummy class into sage.misc.sageinspect, since importing UniqueRepresentation and Parent in sageinspect seems to create an import cycle. Hence, the dummy is now in sage.structure.parent - which should be

Apply trac11768_source_of_dynamic_class.patch

comment:31 follow-up: Changed 7 years ago by nthiery

Hi Simon!

Thanks for your work on finalizing this. What about putting the example in a new separate file. What about putting the test class in a separate file like sage.misc.sageinspect_test.py that would only be imported in the doctest?

Cheers,

Nicolas

comment:32 in reply to: ↑ 31 Changed 7 years ago by SimonKing

Replying to nthiery:

Thanks for your work on finalizing this. What about putting the example in a new separate file. What about putting the test class in a separate file like sage.misc.sageinspect_test.py that would only be imported in the doctest?

Volker and Florent, do you agree with Nicolas that a separate file would be better?

comment:33 follow-up: Changed 7 years ago by vbraun

If you can't find an appropriate class to test it with then I'm fine with an extra file. I would suggest something in sage/tests, though. Maybe sage/tests/sageinspect.py. The file can also test itself so it would really be a test (ooh my head hurts now).

comment:34 in reply to: ↑ 33 ; follow-up: Changed 7 years ago by nthiery

Replying to vbraun:

If you can't find an appropriate class to test it with then I'm fine with an extra file. I would suggest something in sage/tests, though. Maybe sage/tests/sageinspect.py.

I would argue for keeping the tests or test material close to the file under test. We already have sage.misc.nested_class_test which is very similar in purpose. Actually, maybe this should just go there!

The file can also test itself so it would really be a test (ooh my head hurts now).

:-)

comment:35 in reply to: ↑ 34 Changed 7 years ago by SimonKing

Replying to nthiery:

I would argue for keeping the tests or test material close to the file under test. We already have sage.misc.nested_class_test which is very similar in purpose. Actually, maybe this should just go there!

Good idea! I plan this for tomorrow.

comment:36 Changed 7 years ago by SimonKing

OK, done, ready for review!

Not that the coverage script complains. But I think the coverage script has a bug, since it reports:

simon@linux-sqwp:~/SAGE/debug/sage-5.5.rc0/devel/sage> ../../sage -coverage sage/misc/sageinspect.py
----------------------------------------------------------------------
sage/misc/sageinspect.py
ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE sage/misc/sageinspect.py: 60% (26 of 43)

Missing documentation:
         * foo(x, a=')"', b={not (2+1==3):
         * __init__(self):
         * tokeneater(self, type, token, srow_scol, erow_ecol, line):
         * foo(x, a=')"', b={(2+1):
         * foo(x, a='\')"', b={not (2+1==3):
         * dummy' + _grep_first_pair_of_parentheses(source) + ':\n return' try: return _sage_getargspec_from_ast(proxy) except SyntaxError: # To fix trac #10860. See #11913 for more information. return None elif isinstance(obj,functools.partial):
         * test_funct(x,y):
         * create_set_partition_function(letter, k):
         * __cinit__(self):
         * object pyobject(self):
         * __init__(self, ring, gens, coerce=True):
         * test1(a, b=2):
         * test3(b, # 12 a=2):
         * __init__(self, x=None, base=0):
         * test1(a, b=2):
         * test3(b, # 12 a=2):


Missing doctests:
         * _getblock(lines):


Possibly wrong (function name doesn't occur in doctests):
         * dummy' + source[beg:] + '\n return' return tuple(_sage_getargspec_from_ast(proxy)) except Exception: try: # Try to parse just the arguments as a Python argspec. proxy = 'def dummy' + _grep_first_pair_of_parentheses(source) + ':\n return' return tuple(_sage_getargspec_from_ast(proxy)) except Exception: raise ValueError, "Could not parse cython argspec" def sage_getfile(obj):

First bug: It expects a TestSuite.run() test, but there is no class defined for which this would make sense.

Second bug: The dummy' + source[beg:] + '\n return' return tuple(_sage_getargspec_from_ast(proxy))... thingy.

Apply trac11768_source_of_dynamic_class.patch

comment:37 Changed 7 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to import location for nested test class

Oops, I just notice that in one test I still try to import the testclass from sage.structure.parent. Needs a rather trivial fix...

Changed 7 years ago by SimonKing

Get source lines of dynamic classes (e.g., parent_class of a category). Combined patch

comment:38 Changed 7 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues import location for nested test class deleted

Should now be fixed!

Apply trac11768_source_of_dynamic_class.patch

comment:39 Changed 7 years ago by vbraun

Looks good to me.

The coverage script is going crazy. But the correct fix would be to replace the regex cludge with a python lexer/parser. Definitely material for another ticket...

comment:40 Changed 7 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:41 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.