#8420: new feature : class of perfect matching
------------------------------+---------------------------------------------
Reporter: vferay | Owner: sage-combinat
Type: enhancement | Status: needs_work
Priority: major | Milestone:
Component: combinatorics | Keywords: perfect matching
Author: Valentin Feray | Upstream: N/A
Reviewer: Florent Hivert | Merged:
Work_issues: |
------------------------------+---------------------------------------------
Changes (by hivert):
* status: needs_review => needs_work
Comment:
Hi Valentin,
I've got a some comments about your code ;-). I've already taken care of
most of them. For the other I need you. I've uploaded here the patch with
all my modifications and put it also in the Sage-Combinat queue. You
should formally review this patch, and then either fold it in yours (using
in the queue
{{{
sage -hg qgoto your_patch
sage -hg qfold my_patch
sage -b
}}}
) add your new modification or add the new modification in a new patch
after mine. Please ask me if you have some trouble with that.
== Problems already taken care of ==
Please review the patch to see if you agree with all those.
- (done) You should put a title to the file and insert it in the doc
{{{doc/en/reference/combinat/index.rst}}}
- (done) You should put the imports at the beginning of the file, unless
you have a good reason not to do so (eg:loop in import).
- (done) You should avoid too lines in comment, code and documentation.
Please break them at 80 columns.
- (done) Every single function must be tested.
- (done, must be reread) {{{`...`}}} is for math, {{{``...``}}} is for
code.
- (done) You can add cross references to other method with
{{{:meth:`name_of_the_method`}}}
- (done) I don't like the way your wrote three time (or probably copy-
paste) the code for computing crossing/number_of_crossing/is_non_crossing.
A good way it to write an iterator. An even better way is to write yet a
new {{{EnumeratedSet}}} for the crossing.
== Work still to be done ==
- Just after the title, you should define mathematically in one or two
lines what is a perfect matching and add some references
- You should also then add yourself as the author of the file (please see
the [http://www.sagemath.org/doc/developer/conventions.html coding
conventions] and in particular the
[http://www.sagemath.org/doc/developer/conventions.html#headings-of-sage-
library-code-files heading section] of the developper guide).
- in method {{{loop_type}}}, It would probably be useful to expose a
method called {{{loops}}} since you compute it inside. I can imagine it
could be of use for some problems. Then of course you should use it in
{{{loop_type}}}.
- I don't like the following behavior. Is there any good reason for it:
{{{
The class constructor does not check that the perfect matching is
correct,
one has to use the method :meth:`__contains__` for that::
sage: m=PerfectMatching([(1,2,3),(4,5)])
sage: isinstance(m,PerfectMatching)
True
sage: m in m.parent()
False
}}}
- Could you add some reference about perfect-matching and
Weingarten_matrix
- Could you finally add some more tests and in particular corner cases
(empty or odd size sets).
== Final Comments ==
Sorry for this frightening review. Despite of the bunch of remarks, the
code is good ! I'd just rather it to match perfectly with sage standard
;-)
Florent
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/8420#comment:6>
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 post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
