Opened 11 years ago

Closed 9 years ago

#12895 closed enhancement (fixed)

Categories: adds support for SubcategoryMethods

Reported by: Nicolas M. Thiéry Owned by: Nicolas M. Thiéry
Priority: major Milestone: sage-5.11
Component: categories Keywords:
Cc: Sage Combinat CC user, Simon King Merged in: sage-5.11.beta2
Authors: Nicolas M. Thiéry Reviewers: Simon King
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #9138, #11900, #11943, #12875, #12876, #12877, #11935, #12894 Stopgaps:

Status badges

Description (last modified by Simon King)

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

Attachments (3)

trac_12895-subcategory-methods-nt.patch (24.1 KB) - added by Nicolas M. Thiéry 10 years ago.
Sage_crash_report.txt (35.4 KB) - added by Simon King 10 years ago.
Crashlog
trac_12895-review.patch (3.1 KB) - added by Simon King 10 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 11 years ago by Simon King

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.

comment:2 in reply to:  1 Changed 11 years ago by Nicolas M. Thiéry

Replying to SimonKing:

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.

comment:3 Changed 11 years ago by Nicolas M. Thiéry

Status: newneeds_review

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

Changed 10 years ago by Nicolas M. Thiéry

comment:4 Changed 10 years ago by Nicolas M. Thiéry

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

comment:5 Changed 10 years ago by Nicolas M. Thiéry

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

comment:6 in reply to:  5 ; Changed 10 years ago by Anne Schilling

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

Anne

comment:7 in reply to:  6 Changed 10 years ago by Nicolas M. Thiéry

Replying to aschilling:

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 :-)

comment:8 Changed 10 years ago by Simon King

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.

Last edited 10 years ago by Simon King (previous) (diff)

comment:9 Changed 10 years ago by Simon King

The patch has a couple of old style line continuations.

Changed 10 years ago by Simon King

Attachment: Sage_crash_report.txt added

Crashlog

comment:10 Changed 10 years ago by Simon King

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

comment:11 Changed 10 years ago by Simon King

Status: needs_reviewneeds_work

comment:12 Changed 10 years ago by Simon King

Status: needs_workneeds_review

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

comment:13 in reply to:  8 Changed 10 years ago by Nicolas M. Thiéry

Replying to SimonKing:

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.

Changed 10 years ago by Simon King

Attachment: trac_12895-review.patch added

comment:14 Changed 10 years ago by Simon King

Description: modified (diff)
Reviewers: Simon King
Status: needs_reviewpositive_review

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

comment:15 Changed 10 years ago by Nicolas M. Thiéry

For the record: I just checked the review patch.

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

comment:16 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.10sage-5.11

comment:17 Changed 10 years ago by Jeroen Demeyer

Dependencies: #11935, #12894#9138, #11900, #11943, #12875, #12876, #12877, #11935, #12894

comment:18 Changed 9 years ago by Nicolas M. Thiéry

Hi Jeroen!

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

Thanks!

comment:19 in reply to:  18 Changed 9 years ago by Jeroen Demeyer

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.

comment:20 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.11.beta2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.