Opened 12 years ago
Closed 9 years ago
#10193 closed enhancement (fixed)
Create the category of SetsWithGrading
Reported by: | Nicolas M. Thiéry | Owned by: | Sage Combinat CC user |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | combinatorics | Keywords: | categories, enumerated sets, sd35.5, days45 |
Cc: | Sage Combinat CC user, nborie | Merged in: | sage-5.11.beta0 |
Authors: | Nicolas M. Thiéry, Vincent Delecroix | Reviewers: | Nicolas Borie, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #6495 #13605 | Stopgaps: |
Description (last modified by )
The patch creates the category SetsWithGrading
(with an example: non negative integers, graded by themselves) and puts WeightedIntegerVectors
into this category.
See also: #10194
Apply:
Attachments (4)
Change History (38)
comment:1 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 12 years ago by
comment:6 follow-up: 7 Changed 11 years ago by
Keywords: | sd35.5 added |
---|---|
Status: | new → needs_info |
Hi, here at SD35.5 I wanted to review #9280, but I see it depends on this ticket, which did not progress since 15 months. Is this ticket dead?
Paul
PS: moreover the patch url is dead...
comment:7 Changed 11 years ago by
Description: | modified (diff) |
---|
Replying to zimmerma:
Hi, here at SD35.5 I wanted to review #9280, but I see it depends on this ticket, which did not progress since 15 months. Is this ticket dead?
We just discussed it with Vincent Delecroix, and where planning to finalize/review it during the Sage-Combinat days in Cernay (early February).
PS: moreover the patch url is dead...
Should be fixed now.
comment:8 Changed 10 years ago by
comment:9 Changed 10 years ago by
Dependencies: | → #13605 |
---|
Note that this will be swapped with #13605 in the combinat queue once Andrew uploads his review patch.
comment:10 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_info → needs_review |
comment:11 Changed 10 years ago by
Summary: | Create the category of GradedEnumeratedSets → Create the category of GradedSets |
---|
comment:12 Changed 10 years ago by
Authors: | Nicolas M. Thiéry → Nicolas M. Thiéry, Vincent Delecroix |
---|
comment:14 Changed 10 years ago by
Cc: | nborie added |
---|---|
Description: | modified (diff) |
Changed 10 years ago by
Attachment: | trac_10193-graded_sets.patch added |
---|
comment:15 Changed 10 years ago by
What's new:
- remove the changes for partitions as it affects #13605
- rename
GradedSets
toSetsWithGrading
- creation of an example (sage.categories.examples.sets_with_grading)
- fix documentation
Vincent
comment:16 Changed 10 years ago by
Summary: | Create the category of GradedSets → Create the category of SetsWithGrading |
---|
comment:17 Changed 10 years ago by
Dependencies: | #13605 → #6495 #13605 |
---|---|
Keywords: | days45 added |
Milestone: | → sage-5.8 |
Hey Vincent,
Let us know when the rebased patch over #6495 is up and ready for review.
Thanks,
Travis
comment:18 Changed 10 years ago by
Hey Travis,
Actually it is (in the sage-combinat queue). I wait for the review patch of Nicolas Borie !
Best, Vincent
comment:19 Changed 10 years ago by
rebased on 5.8.beta4 (just a doc change)
apply trac_10193-graded_sets-rebased.patch
comment:20 follow-up: 21 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_review → needs_work |
Work issues: | → doctest |
There remains at least one self.n which has not been turned into a self._n
This makes a lot of tests fail.
comment:21 Changed 10 years ago by
Replying to chapoton:
There remains at least one self.n which has not been turned into a self._n
This makes a lot of tests fail.
You are right. I also notice that the comment message in the top of the patch file is not adapted to the content (the patch does not touch anymore Permutations).
Changed 10 years ago by
Attachment: | trac_10193-graded_sets-rebased.patch added |
---|
comment:24 Changed 9 years ago by
Status: | needs_work → needs_review |
---|---|
Work issues: | doctest |
comment:25 Changed 9 years ago by
Reviewers: | Jason Bandlow, Franco Saliola, ... → Nicolas Borie, Travis Scrimshaw |
---|---|
Status: | needs_review → needs_work |
Hey,
A few comments/questions:
- I prefer the name
GradedSets
overSetsWithGrading
, it's a more natural name to me. - The example, I don't think there should be the need for another
NonNegativeIntegers
class. Can't we just use the existingsage.sets.NonNegativeIntegers
with the required methods and placed in this category? - Similar to above, I'd like to see all sets which belong actually into this category. Or at least a few more of the big classes.
- Grade should be implemented as an abstract Element method
- I don't like the default implementation of
graded_component()
since it assumes the first argument of subset (which may not be implemented) is the graded component. If you want to use thesubset()
method, you should call it withself.subset(grade=grade)
. - It's not well documented what methods must be implemented. The "automatic" way is to make them
@abstract_method
s.
Here's also some more wish-list type things that I'd like to see, but can wait for a followup ticket:
- Simple API for grading shifts
- Checking that morphisms preserve grading
- Make
degree()
callgrade()
(thus act like an alias, unfortunately in categories you can't simply make an alias bydegree = grade
[it will reference the wrong function]).
Thanks,
Travis
comment:26 follow-up: 27 Changed 9 years ago by
I just talked with Nicolas T. and basically my previous comments are to be disregarded except the documentation of the methods which need to be implemented needs to be improved.
Best,
Travis
comment:27 Changed 9 years ago by
Replying to tscrim:
I just talked with Nicolas T. and basically my previous comments are to be disregarded except the documentation of the methods which need to be implemented needs to be improved.
I should add that those objections were natural! It would be good to explain in the documentation why SetsWithGrading? rather than GradedSets?. But this patch has been waiting long enough and it needs to be finally be put to real use; let's not delay it for this reason.
+1 as well on the feature requests in a later patch! (except for degree=grade for which I'd rather wait until we have some large scale feedback to see if there really would be a desire for it).
comment:28 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Here's a review patch which describes what needs to be implemented and some other minor doc fixes. If you're happy with the changes, you can set this to positive review.
Best,
Travis
For patchbot:
Apply: trac_10193-graded_sets-rebased.patch trac_10193-graded_sets-review-ts.patch
comment:29 Changed 9 years ago by
Hi,
Thanks Travis. I am not happy!! The last sentence of the documentation of the category is uncomprehensible and there should be a link to the example.
I will upload an ultimate patch in a minute...
Vincent
Changed 9 years ago by
Attachment: | trac_10193-graded_sets-more_doc-vd.patch added |
---|
comment:30 Changed 9 years ago by
Description: | modified (diff) |
---|
It also appears that the example was not included in the documentation...
Apply: trac_10193-graded_sets-rebased.patch trac_10193-graded_sets-review-ts.patch trac_10193-graded_sets-more_doc-vd.patch
Changed 9 years ago by
Attachment: | trac_10193-graded_sets-review-ts.patch added |
---|
comment:31 follow-up: 32 Changed 9 years ago by
Description: | modified (diff) |
---|
Hey Vincent,
I've folded your review patch in and made changes to that (very) poorly written sentence I wrote.
Best,
Travis
For patchbot:
Apply: trac_10193-graded_sets-rebased.patch trac_10193-graded_sets-review-ts.patch
comment:32 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_review → positive_review |
Hi Travis,
I've folded your review patch in and made changes to that (very) poorly written sentence I wrote.
Perfect! I am not sure it was yours (and your english is for sure better than mine). I remember the first time I learn how to do a parent and a category: I found useful when they were specifications and pointers ! Best Vincent
comment:33 Changed 9 years ago by
Milestone: | sage-5.10 → sage-5.11 |
---|
comment:34 Changed 9 years ago by
Merged in: | → sage-5.11.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
It seems like it would be better to use degree as opposed to size for consistency with the "standard" graded things. Also, it seems like it might be better to have the degree method implemented on the parents. It could be somewhat confusing if the degree method on the elements corresponded to a different grading than the parent it belongs to.