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:

Status badges

Description (last modified by Vincent Delecroix)

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)

trac_10193-graded_sets.patch (22.2 KB) - added by Vincent Delecroix 10 years ago.
trac_10193-graded_sets-rebased.patch (22.5 KB) - added by Frédéric Chapoton 10 years ago.
trac_10193-graded_sets-more_doc-vd.patch (2.8 KB) - added by Vincent Delecroix 9 years ago.
trac_10193-graded_sets-review-ts.patch (6.1 KB) - added by Travis Scrimshaw 9 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 12 years ago by Nicolas M. Thiéry

Description: modified (diff)

comment:2 Changed 12 years ago by Nicolas M. Thiéry

Description: modified (diff)

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

Description: modified (diff)

comment:4 Changed 12 years ago by Mike Hansen

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.

comment:5 Changed 11 years ago by Florent Hivert

Hi ! What is the status of this one ?

comment:6 Changed 11 years ago by Paul Zimmermann

Keywords: sd35.5 added
Status: newneeds_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 in reply to:  6 Changed 11 years ago by Nicolas M. Thiéry

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 Travis Scrimshaw

Hey Nicolas and Florent (and Vincent),

I was wondering what the status of this patch is since #13605 is currently based on this patch. I can commute #13605 past, but I would prefer to keep the current order (especially if this patch is close to being completed).

Thank you,
Travis

comment:9 Changed 10 years ago by Travis Scrimshaw

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 Vincent Delecroix

Description: modified (diff)
Status: needs_infoneeds_review

comment:11 Changed 10 years ago by Vincent Delecroix

Summary: Create the category of GradedEnumeratedSetsCreate the category of GradedSets

comment:12 Changed 10 years ago by Vincent Delecroix

Authors: Nicolas M. ThiéryNicolas M. Thiéry, Vincent Delecroix

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

TODO: add an example!

comment:14 Changed 10 years ago by Vincent Delecroix

Cc: nborie added
Description: modified (diff)

Changed 10 years ago by Vincent Delecroix

comment:15 Changed 10 years ago by Vincent Delecroix

What's new:

  • remove the changes for partitions as it affects #13605
  • rename GradedSets to SetsWithGrading
  • creation of an example (sage.categories.examples.sets_with_grading)
  • fix documentation

Vincent

comment:16 Changed 10 years ago by Vincent Delecroix

Summary: Create the category of GradedSetsCreate the category of SetsWithGrading

comment:17 Changed 10 years ago by Travis Scrimshaw

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 Vincent Delecroix

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 Frédéric Chapoton

rebased on 5.8.beta4 (just a doc change)

apply trac_10193-graded_sets-rebased.patch

comment:20 Changed 10 years ago by Frédéric Chapoton

Description: modified (diff)
Status: needs_reviewneeds_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 in reply to:  20 Changed 10 years ago by Vincent Delecroix

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

comment:22 Changed 10 years ago by Frédéric Chapoton

I have changed the .n and now tests should pass

Changed 10 years ago by Frédéric Chapoton

comment:23 Changed 10 years ago by Frédéric Chapoton

new patch, removing one assert statement

comment:24 Changed 10 years ago by Frédéric Chapoton

Status: needs_workneeds_review
Work issues: doctest

comment:25 Changed 9 years ago by Travis Scrimshaw

Reviewers: Jason Bandlow, Franco Saliola, ...Nicolas Borie, Travis Scrimshaw
Status: needs_reviewneeds_work

Hey,

A few comments/questions:

  • I prefer the name GradedSets over SetsWithGrading, 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 existing sage.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 the subset() method, you should call it with self.subset(grade=grade).
  • It's not well documented what methods must be implemented. The "automatic" way is to make them @abstract_methods.

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() call grade() (thus act like an alias, unfortunately in categories you can't simply make an alias by degree = grade [it will reference the wrong function]).

Thanks,
Travis

comment:26 Changed 9 years ago by Travis Scrimshaw

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 in reply to:  26 Changed 9 years ago by Nicolas M. Thiéry

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, for example, 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).

Last edited 9 years ago by Nicolas M. Thiéry (previous) (diff)

comment:28 Changed 9 years ago by Travis Scrimshaw

Description: modified (diff)
Status: needs_workneeds_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 Vincent Delecroix

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 Vincent Delecroix

comment:30 Changed 9 years ago by Vincent Delecroix

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 Travis Scrimshaw

comment:31 Changed 9 years ago by Travis Scrimshaw

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 in reply to:  31 Changed 9 years ago by Vincent Delecroix

Description: modified (diff)
Status: needs_reviewpositive_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 Jeroen Demeyer

Milestone: sage-5.10sage-5.11

comment:34 Changed 9 years ago by Jeroen Demeyer

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