#14141: Implementation of Knutson-Tao puzzles
----------------------------------------------------------------------+-----
Reporter: aschilling |
Owner: sage-combinat
Type: enhancement |
Status: needs_review
Priority: major |
Milestone: sage-5.10
Component: combinatorics |
Resolution:
Keywords: puzzles, Littlewood-Richardson coefficiens, days45 | Work
issues:
Report Upstream: N/A |
Reviewers: Allen Knutson, Liz Beazley, Ed Richmond
Authors: Franco Saliola, Avinash Dalal, Anne Schilling |
Merged in:
Dependencies: #14299 |
Stopgaps:
----------------------------------------------------------------------+-----
Comment (by aschilling):
Replying to [comment:10 saliola]:
> General comments on the patch:
> - This patch has not been rebased: it fails to apply on 5.9.beta5 (and
on 5.9.rc0 according to the patchbot).
I think it depends trivially on #14299 which was merged in
sage-5.10.beta0.
> Comments on the interface:
> - I don't think that {{{PuzzlePiece, PuzzlePieces, PuzzleSolver}}}
should be imported into the global namespace. These names are too generic
to apply to only KT puzzles.
How would the user then find information about puzzles? If we do not
import anything in the namespace, then there is no entry point via tab
completion.
> - Is it expected that the average user will create new puzzle pieces? or
is it that {{{PuzzleSolver}}} will be the most used command?
I think {{{PuzzleSolver}}} will be the most common command. And currently
the user does not have to create his/her own puzzle pieces since
{{{PuzzleSolver("H")}}} is an option. That's also why I think
{{{PuzzleSolver}}} should be in the global namespace.
> - I think it would be preferable to have one entry point to the module,
from which users import what they need.
> - And this entry point should have appropriate documentation describing
all the features (kind of like the current documentation to the module,
but with some enhancements).
> - Perhaps {{{KnutsonTaoPuzzleSolver}}}?
Sure, we could rename it this way.
> Comments on documentation to the module (top of the file):
> - If I do {{{import knutson_tao_puzzles}}} followed by
{{{knutson_tao_puzzles?}}}, I see {{{1/0 0/1}}} as a puzzle piece. I
don't know what this piece is. Is this a problem with the documentation
rendering system?
I do not seem to see this!?
> - The string representations should be explained: most people won't know
what {{{1/0 0\1}}} means.
> - The {{{TODO}}} section describes a possible speed up. Isn't this what
{{{_fill_strip}}} and {{{_fill_puzzle_by_strips}}} does?
Ok, then please remove this comment!
> Comments on {{{PartialPuzzle}}}:
> - {{{LR_tab_from_puzzle}}} is not a good name. Perhaps just call it
{{{skew_lr_tableau}}}.
> - {{{plot_thru_puzzle}}} is not a good name. {{{thru}}} is not an
English word.
Ok, feel free to change the names.
> - the error from comment 7 above should be included as a doctest.
> - {{{assert}}} should be avoided.
> - {{{partition_from_string}}}: I don't know what this method does. The
documentation is not descriptive. What is the "string data" of a
partition? If this is a standard partition constructor, then should it be
in {{{Partition}}} instead?
Yes, in fact you can do
{{{
sage: Partitions().from_zero_one([1, 1, 1, 1, 0, 1, 0])
[5, 4]
}}}
But I think the only problem is that one needs the partition to be padded
by 0s.
> Comments on {{{PuzzleSolver}}}:
> - "Initialize the puzzle solver" is not a sufficient 1-line description
since it doesn't mention the kind of puzzles dealt with.
> - there is an option called 'maxLetter' in the INPUT section, but the
init doesn't take maxLetter as an argument ... oh wait, it's in
{{{__classcall_private__}}}. Why is {{{__classcall_private__}}} needed
here?
To mend the input, so that UniqueRepresentation works!
> - there should be doctests that show how to use "maxLetter"
> - The output is not a function (but instances of the class happen to be
callable).
> - several methods are missing the mandatory 1-line string documentation
and instead just include examples / tests.
I think private methods are not required to have the one line description.
> Comments on {{{SchubertCalculusAlgebra}}}:
> - should this really be in this module? it seems to experimental (there
is also some commented out code that shouldn't be included)
Ed and Liz really liked this feature, to be able to compute structure
coefficients in an easy way.
> - it was implemented as a proof of concept and has no special features;
perhaps it should be deleted and further developed in a separate patch?
> - in the doctest for {{{product_on_basis}}}, the base ring of the
algebra is {{{QQ}}}, but the coefficients belong to a different ring:
> {{{
> sage: from sage.combinat.knutson_tao_puzzles import
SchubertCalculusAlgebra
> sage: Sc = SchubertCalculusAlgebra(QQ, (3,3)).basis()
> sage: x = Sc[Word('001011')] * Sc[Word('010101')]
> sage: x.parent().base_ring()
> Rational Field
> sage: x.coefficients()[0].parent()
> Multivariate Polynomial Ring in y0, y1, y2, y3, y4, y5, y6 over Integer
Ring
> }}}
That is indeed a problem!
>I don't have the time to address my comments myself this week with a
proper review patch, but I can at some point over the next week. There are
a few things to decide beforehand, so we can at least discuss these here
in the meantime.
I am looking forward to your review patch next week! I am moving back to
Davis tomorrow, so won't have time either to work on this before your
review patch.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14141#comment:16>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
