-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
Bender-Knuth involutions and standardization of semistandard Young tableaux #14574
Comments
Attachment: trac_14574_preliminary_version.patch.gz not yet merge ready |
This comment has been minimized.
This comment has been minimized.
Attachment: trac_14574-bender_knuth_et_al-v1.patch.gz |
Attachment: trac_14574-bender_knuth_et_al-v1.2.patch.gz Finished version -- please review |
This comment has been minimized.
This comment has been minimized.
comment:3
[trac_14574-bender_knuth_et_al-v1.2.patch] and [trac_14574-bender_knuth_et_al-v1.patch] are the same file; I derped again when uploading. |
Reviewer: Travis Scrimshaw |
comment:4
Hey Darij, I've made some changes in my review patch to the implementations. In particular, here's a comparison of my implementation of the Bender-Knuth inversions. Here's mine:
Yours:
So there's not much difference, but I'd guess mine could be shown (eventually) to be slightly faster on larger sets. Anyways, if you agree with my changes, you can set this to positive review. Thanks, Travis PS - For future reference, you'll need your real name in the authors section. |
Changed author from darij to Darij Grinberg |
This comment has been minimized.
This comment has been minimized.
comment:6
Hi Travis! That was quick.
Oops, I have no idea how I managed to add a second
into the My reasoning behind the Here's something I should have done myself: In the docstring of On line 504 in your revision, replace "self" by "T". On the next line, "revsersed" should be "reversed". On line 515, "True" should be backticked from both sides. Line 576 in your revision: "Bender-Knuth involution" should be " Line 587: "over all Line 591 of revision: Again, "True" needs more backticks. Line 659: Why do you import Lines 663-694: Is the compiler smart enough that replacing all those calls of Line 681: Add whitespaces to
On the docstrings, most of the above stuff applies again (sorry for duplication). Line 885: Why the "skew"? Line 892: why did you add "skew"? Line 938: Could you pass the Good point that the theorems belong into the Examples, not the Tests section. Good job on the docs as well. |
comment:7
This should take care of everything. For the Anyways, if you'd want, you can pull out the involution part as a separate function which returns a list of lists. It's up to you if you're fine with my implementation or to change it. Best, Travis |
Attachment: trac_14574-bender_knuth-review-ts.patch.gz |
just for comparison |
comment:8
Attachment: for_comparison.patch.gz Thanks for the speedy review. I've folded the review with my old file and thrown in a couple more corrections. I guess I'll open a separate ticket for unchecked generation of tableau objects; this makes more sense to solve on a global scale. Here attachment: for_comparison.patch is just the result of folding, without my corrections. |
comment:9
Very minor nitpick, but could you make the commit message more about what the patch does? Once you've done that, you can set this to positive review. Thanks Darij. |
Attachment: trac_14574-folded.patch.txt |
folded version with a few more corrections |
comment:10
Attachment: trac_14574-folded.patch.gz |
This comment has been minimized.
This comment has been minimized.
comment:11
patchbot: apply trac_14574-folded.patch |
Merged: sage-5.10.beta5 |
I've implemented standardization and Bender-Knuth involutions on straight and skew semistandard tableaux. I've also fixed two typos in error strings and copied some methods for straight tableaux into the skew tableaux file (though I believe much more should be done in this direction).
EDIT: Finished, thanks to Travis.
Apply:
CC: @tscrim
Component: combinatorics
Keywords: tableaux, partitions, combinat
Author: Darij Grinberg
Reviewer: Travis Scrimshaw
Merged: sage-5.10.beta5
Issue created by migration from https://trac.sagemath.org/ticket/14574
The text was updated successfully, but these errors were encountered: