Opened 7 years ago
Closed 5 years ago
#7980 closed enhancement (fixed)
Implement generic support for parents with (multiple) realizations
Reported by: | jbandlow | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | categories | Keywords: | Cernay2012 |
Cc: | sage-combinat | Merged in: | sage-5.0.beta11 |
Authors: | Nicolas M. Thiéry | Reviewers: | Simon King, Florent Hivert |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12484, #12464 | Stopgaps: |
Description (last modified by )
This patch implement generic support for parents with (multiple) realizations See:
Sets().WithRealizations().example()
Attachments (1)
Change History (20)
comment:1 Changed 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
- Description modified (diff)
- Summary changed from Move 'Category with concrete representation' code into Sage to Implement generic support for parents with (multiple) realizations
comment:4 Changed 6 years ago by
- Description modified (diff)
- Milestone changed from sage-4.6.1 to sage-4.6.2
- Status changed from new to needs_work
- Type changed from defect to enhancement
comment:5 follow-up: ↓ 6 Changed 5 years ago by
- Work issues set to Implement super_categories for Category_realizations. Wait for 2-categories to be implemented?
Since the ticket description is rather sparse, I'm collecting here some statements that emerged from reading the patch and discussing with Nicolas, who is sitting beside me.
- It disposes of abstract categories. Hooray!
- On the one hand, one has (abstract) vectorspaces. A realization of such abstract vector space is given by specific data, for example by a basis, but it should be emphasized that not all realizations of a vector space are necessarily in terms of a basis. So, not any realization of a vectorspace is a "vectorspace with basis".
- The realizations of a vector space are related by canonical morphisms (for example, change of basis).
- Is any "vectorspace with basis" a realisation of a vector space? No! There is no gurantee that this specific vectorspace with basis is equiped with the above-mentioned canonical morphisms.
TODO
- The docstring of the class
Category_realization
has no examples. Its__init__
method should define its arguments and provide a test._get_name
is not documented either. - Similarly,
Realizations
is undocumented. Realizations.super_categories()
returns[Sets()]
. Idea of Nicolas: Add an optional argumentRealizations(parent, category=Bla)
, so that the default for Bla isSets()
. The given category will be returned as super category.
- I tried some example, with comments/questions inserted:
sage: C = Algebras(QQ) sage: C.WithRealizations() Join of Category of algebras over Rational Field and Category of sets with realizations
Does it really have to be a join category?sage: from sage.categories.category_types import Category_realization sage: Category_realization(QQ) The category of category_realization of Rational Field
So, do I understand correctly that the set of all realisations ofQQ
forms a category, each object being a realisation ofQQ
, the morphisms being canonical maps between the different realisations?sage: Category_realization(QQ).is_subcategory(Rings().WithRealizations()) BOOM sage: hasattr(Category_realization(QQ), 'super_categories') False
So, that's a bug.Category_realization(QQ)
is a category and thus MUST have asuper_categories
method.
Is the category of "Bla with realisations" a 2-category?
Since Nicolas is now teaching, I can not ask him directly:
- If I understand correctly, for any parent P there is a category
Category_realizations(P)
. - For any category C, there is a category
C.WithRealizations()
. - Shouldn't it be the case that for any
P in C
, the realisations of P (thus, the categoryCategory_realizations(P)
) is an object inC.WithRealizations()
? - But if the objects of
C.WithRealizations()
are categories themselves, shouldn't we first implement the notion of higher categories in Sage, before we treat the category of realizations of objects of C?
comment:6 in reply to: ↑ 5 Changed 5 years ago by
- Work issues changed from Implement super_categories for Category_realizations. Wait for 2-categories to be implemented? to Implement super_categories for Category_realizations.
Replying to SimonKing:
Is the category of "Bla with realisations" a 2-category?
No, it isn't. I had a totally wrong notion of a higher category. Sorry.
Anyway, the missing super_categories method must be provided (as for any category).
comment:7 Changed 5 years ago by
- Dependencies set to #12484, #12464
- Keywords Cernay2012 added
comment:8 Changed 5 years ago by
- Description modified (diff)
- Reviewers set to Simon King, Florent Hivert
- Status changed from needs_work to needs_review
- Work issues Implement super_categories for Category_realizations. deleted
100% doctest coverage; all test pass
Category_realization (now Category_realization_of_parent) does not need a super_category method: it is an abstract class.
comment:9 follow-up: ↓ 10 Changed 5 years ago by
Hi Nicolas,
I just posted a review patch on sage-combinat queue. Here are the main changes
- This is mostly documentation, except that I changed trivially the code in
the example, by renaming some parameter names:
- in
SubsetAlgebra(S)
the parameter S is a set;
- in
Fundamental(S)
,In(S)
andOut(S)
it is aSubsetAlgebra
;
I was really confused about that reading the code so that I renamed S
to SAlg
in the second case and added an INPUT:
field in the doc
- The correct markup for see also is the following:
.. seealso:: bla bla bla bla bla
as variants, you can use uppercase as Sphinx markup is not case sensitive. You can also put everything on one line if it fits:
.. SEEALSO:: bla bla bla
If you put a space as in ..see also::
then this become a comment and is
ignored by Sphinx ! This remind me that I should finish #12078 Add an example
of SEE ALSO
- There is a missing doctest in my review patch
sage: A.F() in A.Realizations() Expected: True Got: False
I left it on purpose. I realize that the correct doctest is
sage: A.F() in A.Bases() Expected: True Got: False
But it took me quite a lot of thinking understanding that. I think we should explain this somewhere, probably where the broken test is.
Please review my numerous doc change, fold the patch if you are Ok with it.
Florent
comment:10 in reply to: ↑ 9 Changed 5 years ago by
Replying to hivert:
I just posted a review patch on sage-combinat queue.
Thanks!
I went through it, folded it in, and added a [http://combinat.sagemath.org/patches/file/tip/trac_7980-multiple-realizations-review-nt.patch review review patch]
- This is mostly documentation, except that I changed trivially the code in
the example, by renaming some parameter names:
- in
SubsetAlgebra(S)
the parameter S is a set;
- in
Fundamental(S)
,In(S)
andOut(S)
it is aSubsetAlgebra
;I was really confused about that reading the code so that I renamed
S
toSAlg
in the second case and added anINPUT:
field in the doc
Good point. I ended up renaming SAlg to A since this is what we use in all the examples.
- There is a missing doctest in my review patch
sage: A.F() in A.Realizations() Expected: True Got: False
Ouch! That's not good. I am not sure this is the best approach, but I added A.Realizations() to the super categories Bases(A). Maybe A.Realizations() should be Bases(A) instead. In any cases, that will work for now. I added A._test_realizations() which checks:
R in A.Realizations() for R in A.realizations()
Feel free to fold in my review patch and repost after checking it out.
Cheers,
Nicolas
comment:11 follow-up: ↓ 12 Changed 5 years ago by
Please update the queue first, since I took off the fix to the CategoryObject? link to put it in #9469
comment:12 in reply to: ↑ 11 Changed 5 years ago by
Replying to nthiery:
Please update the queue first, since I took off the fix to the CategoryObject? link to put it in #9469
Again: I fixed a couple trivially failing doctests.
comment:13 in reply to: ↑ description ; follow-up: ↓ 14 Changed 5 years ago by
- Status changed from needs_review to needs_work
- Work issues set to Post the patches on trac
Replying to jbandlow:
This patch implement generic support for parents with (multiple) realizations
What patch? This ticket does not provide any patch.
comment:14 in reply to: ↑ 13 Changed 5 years ago by
Replying to SimonKing:
Replying to jbandlow:
This patch implement generic support for parents with (multiple) realizations
What patch? This ticket does not provide any patch.
I should have mentioned that it is in on the Sage-Combinat queue, and the review by Florent is occuring here. In such situations it's a waste of time to update the patch on trac all the time; and leaving an outdated patch on trac is not so good either (someone might waste time reviewing old stuff).
comment:15 Changed 5 years ago by
For the record: as of version a7993225ce8e/trac_7980-multiple-realizations-nt.patch I'm Ok with the patch upto a few documentation glitches. I pushed the following patch a7993225ce8e/trac_7980-multiple-realizations-review-fh.patch. If you are Ok with my review patch and the tests pass you can set positive review on my behalf. If so don't forget to fold and update.
Florent
Changed 5 years ago by
comment:16 Changed 5 years ago by
- Status changed from needs_work to positive_review
I folded Florent's patch, and we fixed together a couple remaining glitches on the Sage-Combinat queue.
Florent: the sphinx errors were clearly due to a broken sphinx state on my test machine. After nuking the doctrees, this compiled smoothly. So I set a positive review on your behalf.
comment:17 Changed 5 years ago by
One more ! Good !
Florent
comment:18 Changed 5 years ago by
- Work issues Post the patches on trac deleted
comment:19 Changed 5 years ago by
- Merged in set to sage-5.0.beta11
- Resolution set to fixed
- Status changed from positive_review to closed
Didn't we want Realization rather than Representation?