#18003: Implement Fully Packed Loop class
-------------------------------------+-------------------------------------
Reporter: kdilks | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.9
Component: combinatorics | Resolution:
Keywords: fpl, ncp, days64, | Merged in:
days65, asm, lp, fully packed | Reviewers: Jessica Striker,
loop | Travis Scrimshaw
Authors: James Campbell, | Work issues:
Vince Knight, Jessica Striker, | Commit:
Kevin Dilks, Emily Gunawan | 38e19ec52d4b18ebc5725ed68cc75432ce1ac51f
Report Upstream: N/A | Stopgaps:
Branch: |
public/ticket/18003 |
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by tscrim):
* status: needs_review => needs_work
* work_issues: create a parent class =>
Comment:
The coverage failure, I'm no so sure (FYI you can click on the patchbot
info to see details). However the import modules failure is reflecting the
fact that you are not lazily importing this new file (which is there in an
effort to try and keep startup time down). So I would add this line
instead of the straight import:
{{{
lazy_import('sage.combinat.fully_packed_loop', ['FullyPackedLoop',
'FullyPackedLoops'])
}}}
A couple of things about the ticket/code itself:
- `matrix and also extract the link pattern.::` either remove the period
or put a space after it.
- You should not put examples/information for the user in the `__init__`,
instead move it to the class level docstring (for `FullyPackedLoop`). The
`__init__` still needs to have a doctest, and a good one is to typically
create an element `X` and then do `TestSuite(X).run()`, but you already
have this, so leave the `TESTS::` block.
- In `to_alternating_sign_matrix`, remove the blank line at the beginning
of the docstring and start with `Return`.
- You don't need {{{:math:`x = y`}}}, just {{{`x = y`}}} with the
backticks is sufficient. If you want it similar to equation mode in latex,
do
{{{
.. MATH::
x = y
}}}
- The `EXAMPLES::` block in `plot` is indented one to many times.
- For notes, you might want to consider using the `.. NOTE::` block (in
`link_pattern`).
- Period missing in `six_vertex_model` one-line sentence.
- I would make `_vertex_dictionary` a lazy attribute (grep for
`@lazy_attribute`). FYI - This means it's created on-demand and acts like
a regular attribute, so `self._vertex_dictionary[i]` works. Essentially it
is cached, so the downside to this is that the dictionary gets kept alive
as long as that FPL is, but I think each instance is not kept alive too
long. It's up to you.
- Same for `_end_point_dictionary`. In a similar vein, I would remove the
(public) attribute `end_points` and just use `_end_point_dictionary`.
- First line of `FullyPackedLoops` needs a period and an empty line of
space.
- `cardinality` is missing a period at the end of the equation.
- Remove the commented out code in `_element_constructor_`.
- You need to provide a description of `link_pattern`.
- You should describe the bijection with ASMs somewhere, most likely in
the `FullyPackedLoops` (resp. `FullyPackedLoop`) class and add `..
SEEALSO::` to that class in `FullyPackedLoop` (resp. `FullyPackedLoops`)
and `to_fully_packed_loop`.
- The documentation at the top of the file is good, but you can't read it
interactively (only on the online/compiled documentation). I would suggest
moving it to either `FullyPackedLoops` or `FullyPackedLoop`, depending on
which you think is the more common entry point.
- I think the variable name of `current` in `_get_coordinates` is
confusing and you should rename it to something more descriptive.
Looking very good overall, as almost all of these are trivial to minor
tweaks. That's all for now.
--
Ticket URL: <http://trac.sagemath.org/ticket/18003#comment:48>
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.
