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:

GitHub link to the corresponding issue

Description

in particular get rid of CombinatorialClass? there (#12913)

Change History (11)

comment:1 Changed 4 years ago by chapoton

Branch: → u/chapoton/26384
Cc: tscrim added
Commit: → 07177d015db515015c187cc15349a53fa11cbf02
Status: new → needs_review

New commits:

​07177d0get rid of CombinatorialClass for non-decreasing parking functions

comment:2 Changed 4 years ago by chapoton

Maybe I should have kept CombinatorialObject? ?

comment:3 in reply to:  2 Changed 4 years ago by tscrim

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 tscrim

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 chapoton

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 tscrim

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 git

Commit: 07177d015db515015c187cc15349a53fa11cbf02 → 12bca395ae60f37d4a429a5fed0335f5c2443841

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

​12bca39trac 26384 adding hash

comment:8 Changed 4 years ago by chapoton

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

comment:9 Changed 4 years ago by tscrim

Status: needs_review → positive_review

No problem. Thanks for fixing that.

comment:10 Changed 4 years ago by tscrim

Reviewers: → Travis Scrimshaw

comment:11 Changed 4 years ago by vbraun

Branch: u/chapoton/26384 → 12bca395ae60f37d4a429a5fed0335f5c2443841
Resolution: → fixed
Status: positive_review → closed
Note: See TracTickets for help on using tickets.