Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing bug in shuffle product #14121

Closed
sagetrac-chrisjamesberg mannequin opened this issue Feb 14, 2013 · 10 comments
Closed

Fixing bug in shuffle product #14121

sagetrac-chrisjamesberg mannequin opened this issue Feb 14, 2013 · 10 comments

Comments

@sagetrac-chrisjamesberg
Copy link
Mannequin

Shuffle product contains method does not work properly.

sage: w = Word('ab')
sage: x = Word('ac')
sage: w.shuffle(x)
Shuffle product of word: ab and word: ac
sage: s = w.shuffle(x)
sage: s.list()
[word: abac, word: aabc, word: aacb, word: aabc, word: aacb, word: acab]
sage: x*w in s
False

Component: combinatorics

Keywords: shuffle product, days45

Author: Chris Berg

Reviewer: Franco Saliola, Frédéric Chapoton, Nathann Cohen

Merged: sage-5.8.rc0

Issue created by migration from https://trac.sagemath.org/ticket/14121

@sagetrac-chrisjamesberg sagetrac-chrisjamesberg mannequin added this to the sage-5.8 milestone Feb 14, 2013
@sagetrac-chrisjamesberg
Copy link
Mannequin Author

comment:1

Attachment: fixing_shuffle_product_cb.patch.gz

@videlec
Copy link
Contributor

videlec commented Feb 15, 2013

comment:2

Hi,

You should add a test that ensures you actually correct the thing ! However, it would be a good idea to actually rewrite the whole method.

Vincent

@darijgr
Copy link
Contributor

darijgr commented Feb 15, 2013

comment:3

Theorem 7 in http://www8.cs.umu.se/research/uminf/reports/2011/001/part1.pdf seems relevant to finding a polynomial-time algorithm for this. Just mentioning.

@fchapoton
Copy link
Contributor

Attachment: trac_14121_review-fc.patch.gz

@fchapoton
Copy link
Contributor

comment:4

Here is a doctest.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 9, 2013

comment:5

Hellooooooooooooooooooo !!

Well, this patch definitely fixes a bug and is only slower ono the instances for which it gave bad answers... I think that it's good to go ! What would you think of adding a .. TODO in the method's docstring saying that the pdf given above contains the recipe for an efficient future new implementation of that method ?

Whether you chose to add this comment or not, feel free to set this ticket to positive_review once you are set :-)

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 9, 2013

Changed reviewer from Franco Saliola to Franco Saliola, Frédéric Chapoton, Nathann Cohen

@darijgr
Copy link
Contributor

darijgr commented Mar 9, 2013

comment:6

Wait, wait -- I've never said it gives a more efficient solution; I said it "seems relevant". I fear it uses a constant-size alphabet, which is not what we want...

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 10, 2013

comment:7

HMmmmmmmmmmm... Well, then until we find a better way out ... :-)

Nathann

@jdemeyer
Copy link

Merged: sage-5.8.rc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants