Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create the category of SetsWithGrading #10193

Closed
nthiery opened this issue Oct 31, 2010 · 46 comments
Closed

Create the category of SetsWithGrading #10193

nthiery opened this issue Oct 31, 2010 · 46 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Oct 31, 2010

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:

Depends on #6495
Depends on #13605

CC: @sagetrac-sage-combinat @sagetrac-nborie

Component: combinatorics

Keywords: enumerated sets, sd35.5, days45

Author: Nicolas M. Thiéry, Vincent Delecroix

Reviewer: Nicolas Borie, Travis Scrimshaw

Merged: sage-5.11.beta0

Issue created by migration from https://trac.sagemath.org/ticket/10193

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@mwhansen
Copy link
Contributor

mwhansen commented Nov 1, 2010

comment:4

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.

@hivert
Copy link

hivert commented Jun 8, 2011

comment:5

Hi ! What is the status of this one ?

@zimmermann6
Copy link

comment:6

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

@zimmermann6
Copy link

Changed keywords from enumerated sets to enumerated sets, sd35.5

@nthiery
Copy link
Contributor Author

nthiery commented Jan 11, 2012

comment:7

Replying to @zimmermann6:

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.

@nthiery

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Dec 17, 2012

comment:8

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

@tscrim
Copy link
Collaborator

tscrim commented Jan 31, 2013

Dependencies: #13605

@tscrim
Copy link
Collaborator

tscrim commented Jan 31, 2013

comment:9

Note that this will be swapped with #13605 in the combinat queue once Andrew uploads his review patch.

@videlec

This comment has been minimized.

@videlec videlec changed the title Create the category of GradedEnumeratedSets Create the category of GradedSets Feb 13, 2013
@videlec
Copy link
Contributor

videlec commented Feb 13, 2013

Changed author from Nicolas M. Thiéry to Nicolas M. Thiéry, Vincent Delecroix

@nthiery
Copy link
Contributor Author

nthiery commented Feb 14, 2013

comment:13

TODO: add an example!

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Feb 14, 2013

comment:15

Attachment: trac_10193-graded_sets.patch.gz

What's new:

Vincent

@videlec videlec changed the title Create the category of GradedSets Create the category of SetsWithGrading Feb 14, 2013
@tscrim
Copy link
Collaborator

tscrim commented Feb 22, 2013

comment:17

Hey Vincent,

Let us know when the rebased patch over #6495 is up and ready for review.

Thanks,

Travis

@tscrim
Copy link
Collaborator

tscrim commented Feb 22, 2013

Changed keywords from enumerated sets, sd35.5 to enumerated sets, sd35.5, days45

@tscrim
Copy link
Collaborator

tscrim commented Feb 22, 2013

Changed dependencies from #13605 to #6495 #13605

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2013

comment:25

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2013

Changed reviewer from Jason Bandlow, Franco Saliola, ... to Nicolas Borie, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 30, 2013

comment:26

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

@nthiery
Copy link
Contributor Author

nthiery commented Apr 30, 2013

comment:27

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

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Jun 1, 2013

comment:28

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

@videlec
Copy link
Contributor

videlec commented Jun 1, 2013

comment:29

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

@videlec
Copy link
Contributor

videlec commented Jun 1, 2013

@videlec
Copy link
Contributor

videlec commented Jun 1, 2013

comment:30

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

@videlec

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Jun 2, 2013

Attachment: trac_10193-graded_sets-review-ts.patch.gz

@tscrim
Copy link
Collaborator

tscrim commented Jun 2, 2013

comment:31

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

@tscrim

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Jun 2, 2013

comment:32

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

@jdemeyer
Copy link

jdemeyer commented Jun 6, 2013

Merged: sage-5.11.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants