#14456: New methods for alternating sign matrices
-----------------------------------+----------------------------------------
Reporter: jessicapalencia | Owner: sage-combinat
Type: enhancement | Status: new
Priority: major | Milestone: sage-5.10
Component: combinatorics | Resolution:
Keywords: asm | Work issues:
Report Upstream: N/A | Reviewers:
Authors: | Merged in:
Dependencies: #14301 | Stopgaps:
-----------------------------------+----------------------------------------
Comment (by tscrim):
Hey Jessica,
Replying to [comment:4 jessicapalencia]:
> Thanks for the help, Christian. I took care of most of the problems and
cleaned up the code a bit, but when I put the import statement in the
permutation.py file and build sage, it crashes my sage. But it works to
put the same statement in the dyck_word.py file, so I don't know what the
problem is.
That sounds like it's leading to a circular import. That is `A -> B -> C
-> A` where `A -> B` is an import of module `B` in module `A` (of anything
in the module).
A few notes on the code:
* It looks like you are also defining a map from a monotone triangle to a
non-decreasing parking function. It might be a good idea to separate this
out into a method in `MonotoneTriangle`.
* These maps should also have the `@combinatorial_map` decorator so we can
use these (more) effectively in FindStat (correct Christan?).
* `to_DyckWord()` is not a standard python name for a method, it should be
`to_dyck_word()` (you can lobby for `to_Dyck_word()`, and I'll look around
for other maps which capitalize names, but I don't think there are many).
* The documentation is not latex, so things like {{{``self''}}} are not
outputted as "self". Instead these should be formatted as code using the
double backticks: {{{``self``}}}
* You can use latex in the documentation with single backticks, so for
example, line 2631 in Dyck word could be:
{{{
This is an inclusion map from Dyck Words of length `2n` to certain `n
\times n` alternating sign matrices.
}}}
* I can handle some docstring formatting in a review patch too.
* Your doctest for `to_alternating_sign_matrix()` in permutation does not
test that they are in fact ASM's. A good check would be to print their
`parent()`. (Doing things like this will make it easier to spot errors in
the future.)
* You'll need to export your patch with a good commit message.
I'm sorry that seems more like a laundry list, but I'd need to see those
changes before I could let this have a positive review. Feel free to ask
me any questions.
----
What follows is somewhat of a rant (you've been warned).
A note on coding style, I find it very hard to read things like
{{{
PF2=[len(PF)+1-PF[i] for i in range(len(PF))]
}}}
A few spaces around (especial on either sides of assignment `=` equals)
makes it easier to follow IMO. This is a personal preference and its on
the reviewer to deal with whatever style you choose (up to standards of
course), so don't feel that you need to change it.
I should also state that it is a good idea to try and keep line lengths to
80 characters in the documentation parts (code is not so important, but
try not to let them grow too long lines). This makes it display better for
those who have thinner terminal windows (80 has historical reasons as
being a "maximal" line length). Again, not a need to change, but I've seen
a few files which goes double this and beyond.
----
Thanks,[[BR]]
Travis
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/14456#comment:5>
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.
