Opened 4 years ago
Closed 4 years ago
#26384 closed enhancement (fixed)
cleanup non-decreasing parking functions
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.4 |
Component: | combinatorics | Keywords: | |
Cc: | tscrim | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 12bca39 (Commits, GitHub, GitLab) | Commit: | 12bca395ae60f37d4a429a5fed0335f5c2443841 |
Dependencies: | Stopgaps: |
Description
in particular get rid of CombinatorialClass? there (#12913)
Change History (11)
comment:1 Changed 4 years ago by
Branch: | → u/chapoton/26384 |
---|---|
Cc: | tscrim added |
Commit: | → 07177d015db515015c187cc15349a53fa11cbf02 |
Status: | new → needs_review |
comment:3 Changed 4 years ago by
Replying to chapoton:
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.
comment:4 Changed 4 years ago by
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.
comment:5 Changed 4 years ago by
Travis, do you think this could enter sage in the current state ? I would prefer not to spend more time on that..
comment:6 Changed 4 years ago by
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.
comment:7 Changed 4 years ago by
Commit: | 07177d015db515015c187cc15349a53fa11cbf02 → 12bca395ae60f37d4a429a5fed0335f5c2443841 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
12bca39 | trac 26384 adding hash
|
comment:9 Changed 4 years ago by
Status: | needs_review → positive_review |
---|
No problem. Thanks for fixing that.
comment:10 Changed 4 years ago by
Reviewers: | → Travis Scrimshaw |
---|
comment:11 Changed 4 years ago by
Branch: | u/chapoton/26384 → 12bca395ae60f37d4a429a5fed0335f5c2443841 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
get rid of CombinatorialClass for non-decreasing parking functions