Opened 7 years ago
Closed 6 years ago
#12895 closed enhancement (fixed)
Categories: adds support for SubcategoryMethods
Reported by: | nthiery | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | categories | Keywords: | |
Cc: | sage-combinat, SimonKing | 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: |
Description (last modified by )
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)
Change History (23)
comment:1 follow-up: ↓ 2 Changed 7 years ago by
comment:2 in reply to: ↑ 1 Changed 7 years ago by
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.patchbut only get an empty patch.
Strange. Anyway, there it is.
comment:3 Changed 7 years ago by
- Status changed from new to needs_review
Oh, and I should mention that basically all tests pass.
Changed 6 years ago by
comment:4 Changed 6 years ago by
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 follow-up: ↓ 6 Changed 6 years ago by
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 ; follow-up: ↓ 7 Changed 6 years ago by
The changes to the k-Schur function code look ok. Thanks for catching those!
Anne
comment:7 in reply to: ↑ 6 Changed 6 years ago by
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 follow-up: ↓ 13 Changed 6 years ago by
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.
comment:9 Changed 6 years ago by
The patch has a couple of old style line continuations.
comment:10 Changed 6 years ago by
See Sage_crash_report.txt. Sage doesn't even start.
comment:11 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:12 Changed 6 years ago by
- Status changed from needs_work to needs_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 6 years ago by
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 6 years ago by
comment:14 Changed 6 years ago by
- Description modified (diff)
- Reviewers set to Simon King
- Status changed from needs_review to positive_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 6 years ago by
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 6 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:17 Changed 6 years ago by
- Dependencies changed from #11935, #12894 to #9138, #11900, #11943, #12875, #12876, #12877, #11935, #12894
comment:18 follow-up: ↓ 19 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
- Merged in set to sage-5.11.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
How can one get the patch? I tried
but only get an empty patch.