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 )
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 byclass 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)
Change History (42)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
- 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
- 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
- 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
- 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
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
- 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
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: ↓ 10 Changed 8 years ago by
Fails to apply to vanilla sage-4.7.2.alpha2. Dependencies?
comment:10 in reply to: ↑ 9 Changed 8 years ago by
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
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
Sorry, you are right. It does apply. Must have forgotten to pop all patches %-)
comment:13 Changed 8 years ago by
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
Any reviewer for that ticket? I think introspection is a nice thing to have in more generality...
comment:15 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to Make it independent of #11900
comment:16 Changed 8 years ago by
- 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
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
comment:19 Changed 8 years ago by
- 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
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: ↓ 22 Changed 8 years ago by
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: ↓ 23 Changed 8 years ago by
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
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
Any review for better access to source code in the category framework?
comment:25 follow-up: ↓ 26 Changed 7 years ago by
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
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
I think what I need is:
- A parent class whose metaclass is a sub-class of
NestedClassMetaclass
(such asUniqueRepresentation
) - ... 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
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
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
- 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: ↓ 32 Changed 7 years ago by
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
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: ↓ 34 Changed 7 years ago by
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: ↓ 35 Changed 7 years ago by
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. Maybesage/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
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
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
- 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
Get source lines of dynamic classes (e.g., parent_class of a category). Combined patch
comment:38 Changed 7 years ago by
- 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
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
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
comment:41 Changed 7 years ago by
- Merged in set to sage-5.6.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
The patch works exactly as it is announced in the ticket description. One can now (on top of sage-4.7.2.alpha2) do:
Unfortunately,
does not work yet. So, the search for definitions should also include the
__base__
attribute of a class.