#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:                                                      |      
Stopgaps:                                         
----------------------------------------------------------------------+-----

Comment (by 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).
 - All tests pass on 5.9.beta5.
 - some methods are missing {{{INPUT}}} blocks, some do not describe all
 the possible input arguments, some do not test all the possible input
 options

 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.
 - 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 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}}}?

 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?
 - 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?

 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.
 - 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?

 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?
 - 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.

 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)
 - 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
 }}}

-- 
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14141#comment:10>
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.


Reply via email to