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

cleanup non-decreasing parking functions #26384

Closed
fchapoton opened this issue Oct 2, 2018 · 14 comments
Closed

cleanup non-decreasing parking functions #26384

fchapoton opened this issue Oct 2, 2018 · 14 comments

Comments

@fchapoton
Copy link
Contributor

in particular get rid of CombinatorialClass there (#12913)

CC: @tscrim

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: 12bca39

Reviewer: Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-8.4 milestone Oct 2, 2018
@fchapoton
Copy link
Contributor Author

New commits:

07177d0get rid of CombinatorialClass for non-decreasing parking functions

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/26384

@fchapoton
Copy link
Contributor Author

Commit: 07177d0

@fchapoton
Copy link
Contributor Author

comment:2

Maybe I should have kept CombinatorialObject ?

@tscrim
Copy link
Collaborator

tscrim commented Oct 2, 2018

comment:3

Replying to @fchapoton:

Maybe I should have kept CombinatorialObject ?

That one is a maybe. The biggest issue is they are no longer hashable:

sage: hash(NonDecreasingParkingFunction([]))
11648069979105038

which is an error on your branch. It is possible that someone is using the other list-like features of CombinatorialObject not already reimplemented (mainly the __add__ being concatenation), but I find that highly unlikely.

@tscrim
Copy link
Collaborator

tscrim commented Oct 2, 2018

comment:4

IIRC, Nicolas has told me we should use ClonableArray instead of CombinatorialObject/Element. Although I do not like some of the features of the former (mainly, the hash and equality depends on the parent, which can cause lots of subtle problems with objects that live in multiple incompatible (wrt to coercion) parents). Here, however, this would be a reasonable thing to inherit from.

@fchapoton
Copy link
Contributor Author

comment:5

Travis, do you think this could enter sage in the current state ? I would prefer not to spend more time on that..

@tscrim
Copy link
Collaborator

tscrim commented Oct 3, 2018

comment:6

I think it is probably better in the long run to not use CombinatorialObject. However, it does need a __hash__. Other than that, it is good to go.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

12bca39trac 26384 adding hash

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2018

Changed commit from 07177d0 to 12bca39

@fchapoton
Copy link
Contributor Author

comment:8

ok, I have added the hash. Thanks a lot for your help.

@tscrim
Copy link
Collaborator

tscrim commented Oct 3, 2018

comment:9

No problem. Thanks for fixing that.

@tscrim
Copy link
Collaborator

tscrim commented Oct 3, 2018

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Oct 4, 2018

Changed branch from u/chapoton/26384 to 12bca39

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

3 participants