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
Change History (15)
comment:1 Changed 2 years ago by
| Branch: | → u/chapoton/30962 |
|---|---|
| Commit: | → 3fb5611f5a6cee0133b4eaef5c93762e26dfc30e |
| Status: | new → needs_review |
comment:2 Changed 2 years ago by
| Commit: | 3fb5611f5a6cee0133b4eaef5c93762e26dfc30e → 1b9027219a5c0c436d983ae2b8f1399600abc5f6 |
|---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
| 1b90272 | use parent for shuffle.py
|
comment:3 Changed 2 years ago by
| Commit: | 1b9027219a5c0c436d983ae2b8f1399600abc5f6 → 05240d7b90851ef502e7f78bbd64448a6e1a95d1 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 05240d7 | add doctests
|
comment:5 Changed 2 years ago by
| Cc: | tscrim added |
|---|
comment:6 Changed 2 years ago by
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
| Commit: | 05240d7b90851ef502e7f78bbd64448a6e1a95d1 → f8c74550df2d9960638dd3a42bfff2ab20d406fa |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| f8c7455 | adding eq and ne
|
comment:8 Changed 2 years ago by
I have taken care of the pickling. But I have not clue on how to handle the facade property.
comment:10 Changed 2 years ago by
| Branch: | u/chapoton/30962 → public/combinat/shuffle_improvements-30962 |
|---|---|
| Commit: | f8c74550df2d9960638dd3a42bfff2ab20d406fa → aecabd74efbf694230e1ffa3e1ae1c9ae0d0f739 |
| 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:
| 58b8bd3 | Merge branch 'u/chapoton/30962' of git://trac.sagemath.org/sage into u/chapoton/30962
|
| aecabd7 | Better class structure and fixing a few fringe issues with shuffles.
|
comment:11 Changed 2 years ago by
| Commit: | aecabd74efbf694230e1ffa3e1ae1c9ae0d0f739 → 61f8a45abcee012476f126f81edf0d37ddf2e769 |
|---|
Branch pushed to git repo; I updated commit sha1. New commits:
| 61f8a45 | remove unused imports
|
comment:13 follow-up: 14 Changed 2 years ago by
ok, looks good to me. One should enhance the _contains_ in a later ticket..
You can set to positive if you wish
comment:14 Changed 2 years ago by
| Status: | needs_review → positive_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
| Branch: | public/combinat/shuffle_improvements-30962 → 61f8a45abcee012476f126f81edf0d37ddf2e769 |
|---|---|
| Resolution: | → fixed |
| Status: | positive_review → closed |
New commits:
trac 30925 let MZV copy their input if neededuse Parent for shuffle, get rid of CombinatorialClass there