Opened 15 months ago

Closed 15 months 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) Commit: 12bca395ae60f37d4a429a5fed0335f5c2443841
Dependencies: Stopgaps:

Description

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

Change History (11)

comment:1 Changed 15 months ago by chapoton

  • Branch set to u/chapoton/26384
  • Cc tscrim added
  • Commit set to 07177d015db515015c187cc15349a53fa11cbf02
  • Status changed from new to needs_review

New commits:

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

comment:2 follow-up: Changed 15 months ago by chapoton

Maybe I should have kept CombinatorialObject? ?

comment:3 in reply to: ↑ 2 Changed 15 months 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 15 months 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 15 months 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 15 months 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 15 months ago by git

  • Commit changed from 07177d015db515015c187cc15349a53fa11cbf02 to 12bca395ae60f37d4a429a5fed0335f5c2443841

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

​12bca39trac 26384 adding hash

comment:8 Changed 15 months ago by chapoton

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

comment:9 Changed 15 months ago by tscrim

  • Status changed from needs_review to positive_review

No problem. Thanks for fixing that.

comment:10 Changed 15 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

comment:11 Changed 15 months ago by vbraun

  • Branch changed from u/chapoton/26384 to 12bca395ae60f37d4a429a5fed0335f5c2443841
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.