Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Categories: adds support for SubcategoryMethods #12895

Closed
nthiery opened this issue May 1, 2012 · 21 comments
Closed

Categories: adds support for SubcategoryMethods #12895

nthiery opened this issue May 1, 2012 · 21 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented May 1, 2012

With this patch, a category can implement a nested class SubcategoryMethods that provides methods for all subcategories of this category (similar to ParentMethods that provides methods for all parents of all subcategories of this category).

This is implemented by updating the class of each category C, at the end of its initialization, to insert C.subcategory_class as superclass (like what is done for Parents and Elements).

This is a bit tricky, since the super_categories method needs to be called during the initialization.

The patch is under finalization on http://combinat.sagemath.org/patches/

Apply

Depends on #9138
Depends on #11900
Depends on #11943
Depends on #12875
Depends on #12876
Depends on #12877
Depends on #11935
Depends on #12894

CC: @sagetrac-sage-combinat @simon-king-jena

Component: categories

Author: Nicolas M. Thiéry

Reviewer: Simon King

Merged: sage-5.11.beta2

Issue created by migration from https://trac.sagemath.org/ticket/12895

@nthiery nthiery added this to the sage-5.10 milestone May 1, 2012
@nthiery nthiery self-assigned this May 1, 2012
@simon-king-jena
Copy link
Member

comment:1

How can one get the patch? I tried

hg qimport http://combinat.sagemath.org/patches/file/2b30dc3e4bf4/trac_12895-subcategory-methods-nt.patch

but only get an empty patch.

@nthiery
Copy link
Contributor Author

nthiery commented May 3, 2012

comment:2

Replying to @simon-king-jena:

How can one get the patch? I tried

hg qimport http://combinat.sagemath.org/patches/file/2b30dc3e4bf4/trac_12895-subcategory-methods-nt.patch

but only get an empty patch.

Strange. Anyway, there it is.

@nthiery
Copy link
Contributor Author

nthiery commented May 3, 2012

comment:3

Oh, and I should mention that basically all tests pass.

@nthiery
Copy link
Contributor Author

nthiery commented May 22, 2013

comment:4

Attachment: trac_12895-subcategory-methods-nt.patch.gz

Hi Simon,

I have just been through the patch on the Sage-Combinat queue (same as here previously, but rebased a couple times). I improved a bit the documentation and fixed three remaining doctests failures and reposted. On my side, it's good to go. Please review!

Note: the patch includes two small fixes in sage.combinat.sf which I am going to ask Anne to double check.

Cheers,
Nicolas

@nthiery
Copy link
Contributor Author

nthiery commented May 22, 2013

comment:5

For the record: all long tests passed with the following patches applied on top of 5.10 beta4 on Ubuntu:

trac_14612-gyw_test_speedup-ts.patch
trac_14574-folded.patch
trac_14610-LSenergy-ms.patch
try-as.patch
trac9107_nesting_nested_classes.patch
trac_9107_fix_cross_reference.patch
trac_12876_category_abstract_classes_for_hom.patch
trac_12848-posets-order_ideal_complement_generators_fix-nt-v2.patch
trac_14266_ascii_art_13_05_15_EliX-jbp.patch
trac_14266-ascii_art-review-ts.patch
trac11935_weak_pickling_by_construction-nt.patch
trac_11935-weak_pickling_by_construction-review-ts.patch
trac_12895-subcategory-methods-nt.patch

@anneschilling
Copy link

comment:6

The changes to the k-Schur function code look ok. Thanks for catching those!

Anne

@nthiery
Copy link
Contributor Author

nthiery commented May 23, 2013

comment:7

Replying to @anneschilling:

The changes to the k-Schur function code look ok. Thanks for catching those!

Thanks Anne for the review.

Simon: the ticket's review is all yours now :-)

@simon-king-jena
Copy link
Member

comment:8

I see that for class Category_singleton(Category), you replace

    @lazy_class_attribute
    def __classcall__(object cls):

by

    @staticmethod
    def __classcall__(object cls):

The point is that you do obj.__class__._set_classcall(ConstantFunction(obj)), so that the lazy class attribute is not needed, right? Just to make sure I understand what you did.

@simon-king-jena
Copy link
Member

comment:9

The patch has a couple of old style line continuations.

@simon-king-jena
Copy link
Member

Crashlog

@simon-king-jena
Copy link
Member

comment:10

Attachment: Sage_crash_report.txt

See attachment: Sage_crash_report.txt. Sage doesn't even start.

@simon-king-jena
Copy link
Member

comment:12

Never mind. I forgot one dependency (it said "fixed", but #12894 is only fixed in sage-5.10.beta1, which I don't have).

Apply trac_12895-subcategory-methods-nt.patch

@nthiery
Copy link
Contributor Author

nthiery commented May 24, 2013

comment:13

Replying to @simon-king-jena:

I see that for class Category_singleton(Category), you replace

    @lazy_class_attribute
    def __classcall__(object cls):

by

    @staticmethod
    def __classcall__(object cls):

The point is that you do obj.__class__._set_classcall(ConstantFunction(obj)), so that the lazy class attribute is not needed, right? Just to make sure I understand what you did.

That's right. If I remember well (that was one year ago), the tricky part is to have the classcall setting work properly both for the original class and its dynamic subclass. And having __classcall__ be a static method deviates a bit less from the standard UniqueRepresentation idiom.

@simon-king-jena
Copy link
Member

Reviewer: Simon King

@simon-king-jena

This comment has been minimized.

@simon-king-jena
Copy link
Member

comment:14

Attachment: trac_12895-review.patch.gz

Tests pass for me. The patchbot timed out, so I just kicked it. The patch looks fine, except for the line continuation. Hence, I provided a review patch.

Apply trac_12895-subcategory-methods-nt.patch trac_12895-review.patch

@nthiery
Copy link
Contributor Author

nthiery commented May 25, 2013

comment:15

For the record: I just checked the review patch.

Thanks Simon; one more patch done! Now the ball is on my side :-)

@jdemeyer jdemeyer removed this from the sage-5.10 milestone May 27, 2013
@jdemeyer jdemeyer added this to the sage-5.11 milestone May 27, 2013
@jdemeyer
Copy link

Changed dependencies from #11935, #12894 to #9138, #11900, #11943, #12875, #12876, #12877, #11935, #12894

@nthiery
Copy link
Contributor Author

nthiery commented Jun 11, 2013

comment:18

Hi Jeroen!

Wow lots of things merged lately. Thanks! Any chance to get this one as well in beta1?

Thanks!

@jdemeyer
Copy link

comment:19

Replying to @nthiery:

Hi Jeroen!

Wow lots of things merged lately. Thanks! Any chance to get this one as well in beta1?

I don't know. In any case, it's all very academic since Sage 5.10 hasn't been released yet.

@jdemeyer
Copy link

Merged: sage-5.11.beta2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants