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: |
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 needed
use Parent for shuffle, get rid of CombinatorialClass there