#19160: Add a new class SnakeGraphs and SnakeGraph in
combinat/cluster_algebra_quiver
-------------------------------------+-------------------------------------
       Reporter:  egunawan           |        Owner:
           Type:  enhancement        |       Status:  needs_work
       Priority:  major              |    Milestone:  sage-6.9
      Component:  combinatorics      |   Resolution:
       Keywords:  snake graph,       |    Merged in:
  cluster algebra, days69            |    Reviewers:  Travis Scrimshaw,
        Authors:  egunawan           |  mlapointe
Report Upstream:  N/A                |  Work issues:
         Branch:  u/egunawan/19160   |       Commit:
   Dependencies:                     |  dd7241b32660b018c8575e80d31e5fbf6d1cf922
                                     |     Stopgaps:
-------------------------------------+-------------------------------------
Changes (by tscrim):

 * status:  needs_review => needs_work
 * reviewer:  tscrim, mlapointe => Travis Scrimshaw, mlapointe


Comment:

 Thank you for your work on this.

 Some things that need to be addressed from a quick glance:

 - Full doctest coverage (`shape`).
 - `print 'print an error message here TODO'` in `_element_constructor_`
 needs to raise an actual error.
 - `The snake graphs` -> `Snake graphs`.
 - You should include more information in the `SnakeGraphs`'s class level
 docstring.
 - Remove the module variables `RIGHT` and `UP`; just use the python
 strings (I've never noticed a speed decrease; <technobabble>in fact,
 python probably keeps string instances in a pool and/or has special
 compiler behavior for hardcoded strings</technobabble>).
 - You might consider using `ClonableArray` instead of `Element` since the
 former gives you list-like behavior for free (and by handling the internal
 storage in python, will be faster) and is a subclass of `Element`.
 - You should explain the association between your internal data structure
 and the behavior of the snake graph (i.e., what integer makes the graph go
 in which direction?). This can be in python comments, not necessarily in
 the docstrings.

 Looks pretty, and I like the ascii art.

 For the ticket on trac, you should use real names for author and reviewer.

--
Ticket URL: <http://trac.sagemath.org/ticket/19160#comment:4>
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.
For more options, visit https://groups.google.com/d/optout.

Reply via email to