Opened 2 years ago

Closed 2 years ago

#30962 closed enhancement (fixed)

use Parent in shuffle

Reported by: chapoton Owned by:
Priority: major Milestone: sage-9.3
Component: combinatorics Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 61f8a45 (Commits, GitHub, GitLab) Commit: 61f8a45abcee012476f126f81edf0d37ddf2e769
Dependencies: Stopgaps:

Status badges

Description

and get rid there of CoombinatorialClass?

also make the cardinality an integer

part of #12913

Change History (15)

comment:1 Changed 2 years ago by chapoton

Branch: u/chapoton/30962
Commit: 3fb5611f5a6cee0133b4eaef5c93762e26dfc30e
Status: newneeds_review

New commits:

1439dd6trac 30925 let MZV copy their input if needed
3fb5611use Parent for shuffle, get rid of CombinatorialClass there

comment:2 Changed 2 years ago by git

Commit: 3fb5611f5a6cee0133b4eaef5c93762e26dfc30e1b9027219a5c0c436d983ae2b8f1399600abc5f6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1b90272use parent for shuffle.py

comment:3 Changed 2 years ago by git

Commit: 1b9027219a5c0c436d983ae2b8f1399600abc5f605240d7b90851ef502e7f78bbd64448a6e1a95d1

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

05240d7add doctests

comment:4 Changed 2 years ago by chapoton

green bot, please review. Should I care for loads(dumps()) ?

comment:5 Changed 2 years ago by chapoton

Cc: tscrim added

comment:6 Changed 2 years ago by tscrim

I think it would be good to have a little care for pickling. However, I think there is a bigger issue with the fact that these are parents, but they do not have an element class associated to them. They are really facade parents, and I think should be indicated as such.

comment:7 Changed 2 years ago by git

Commit: 05240d7b90851ef502e7f78bbd64448a6e1a95d1f8c74550df2d9960638dd3a42bfff2ab20d406fa

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

f8c7455adding eq and ne

comment:8 Changed 2 years ago by chapoton

I have taken care of the pickling. But I have not clue on how to handle the facade property.

comment:9 Changed 2 years ago by tscrim

I will take care of it either today or tomorrow.

comment:10 Changed 2 years ago by tscrim

Branch: u/chapoton/30962public/combinat/shuffle_improvements-30962
Commit: f8c74550df2d9960638dd3a42bfff2ab20d406faaecabd74efbf694230e1ffa3e1ae1c9ae0d0f739
Reviewers: Travis Scrimshaw

Okay, I couldn't get the facade to work as well because it wants an actual parent. So we have an abuse because we are creating elements that are not actually an Element to the Parent. Hence the skipped TestSuite tests. I reworked the class hierarchy there to have a common ABC and fixed issues with pickling and equality for all of them.


New commits:

58b8bd3Merge branch 'u/chapoton/30962' of git://trac.sagemath.org/sage into u/chapoton/30962
aecabd7Better class structure and fixing a few fringe issues with shuffles.

comment:11 Changed 2 years ago by git

Commit: aecabd74efbf694230e1ffa3e1ae1c9ae0d0f73961f8a45abcee012476f126f81edf0d37ddf2e769

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

61f8a45remove unused imports

comment:12 Changed 2 years ago by tscrim

Whoops, thank you. Green patchbot.

comment:13 Changed 2 years ago by chapoton

ok, looks good to me. One should enhance the _contains_ in a later ticket..

You can set to positive if you wish

comment:14 in reply to:  13 Changed 2 years ago by tscrim

Status: needs_reviewpositive_review

Replying to chapoton:

ok, looks good to me. One should enhance the _contains_ in a later ticket..

Indeed, that would be good. This was not previously supported before I believe.

You can set to positive if you wish

Thank you.

comment:15 Changed 2 years ago by vbraun

Branch: public/combinat/shuffle_improvements-3096261f8a45abcee012476f126f81edf0d37ddf2e769
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.