#8420: new feature : class of perfect matching
------------------------------+---------------------------------------------
Reporter: vferay | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone:
Component: combinatorics | Keywords: perfect matching
Author: Valentin Feray | Upstream: N/A
Reviewer: Florent Hivert | Merged:
Work_issues: |
------------------------------+---------------------------------------------
Comment(by hivert):
Hi Valentin,
First of all, please have a look at
http://trac.sagemath.org/sage_trac/wiki/WikiFormatting
so that your message appear nicely in trac (sorry yet another stupid
syntax to learn)...
> Thanks for this new review. The patch seems ready, except for two small
things:
>
> - line 144 of the file perfect_matching.py, it is written in the doc
{{{`PerfectMatchings(objects)(data)`.}}}
> Should it not be
{{{``PerfectMatchings(objects)(data)``}}}
> as this is sage code?
You are perfectly right ! Thanks for spotting this little mistake.
> - second: the function now called
> hyperoctahedral_double_coset_type
> (which name is fine like this by the way)
> does not return a PerfectMatching as you suggest in the doc,
> but a partition.
Again right...
> As I am learning, I would like to know what is the best way to deal with
very small changes like that:
> - to do a new review patch as in the case of big changes?
Usually when you spot those kind a little mistakes, it takes as long as
explaining them in trac than fixing them directly in the file. So the
fastest and time saving way (for the sage community in general, maybe not
for you) is to write yet another tiny review patch and put in on trac. If
the change is bigger or more involved, you can ask the author to do it.
That's when you check the need work button.
In the present case, since I understood it wrong, can you give a little
explanation of this coset stuff and correct it as a being a partition. If
it's too complicated explaining it, keeping only the ref to Macdonald (as
it is now) can be sufficient. Also, since I spend times explaining this to
you, It's your turn to make the change (sorry that I found a reason for
being lazy ;-).
Then the usual way is to submit yet another review of review patch and ask
for a review of the review of the review patch ;-) Looks like a joke but
this is not.
> - to let you do these changes and switch to positive review?
It's forbidden by rule 2 below. If I change my patch, you have to review
it again.
> - to do the changes myself and switch to positive review? (but I believe
this is not a good idea as I am not supposed to write in your patch)
Again forbidden by both rule 1 and 2 below.
Two strict rules:
1 - in the path queue of sage-combinat, it's forbidden to write on
someone's else patch, except if explicitely authorized (unless you're fond
of editing second derivative of a file)... In some very rare occasion, we
broke this rule with Nicolas but you really have to know what you are
doing... A much better way is to write a new patch and either to submit it
to trac or to ask the owner to fold it.
2 - No code (or doc or anything) can go into Sage, unless it has been
reviewed by someone different from its author. Even a trivial typo
correction. As far as I know since a few year, this rule has never been
broken.
Also it's easier for the different review to keep the consecutive changes
in different patches. An the end, for the release manager, you have to
indicate which patch has to be applied in which order. If you want to be
kind with him. after deciding to put the positive review, you can fold the
whole bunch of patch in a single one (useful when the review has been
particularily messy).
> Another thing that I did not know how to do was the following:
> I remember that you said something about compiling the documentation to
check that it is well written. How does it work?
I must suggest you to read the
[http://www.sagemath.org/doc/developer/index.html Developer's Guide] in
particular the section reviewing a patch (the command about applying the
patch don't strictly apply to our case since we are using the queue).
The answer of you question is found in point 4:
{{{
./sage -docbuild reference html
}}}
could take some time.
Cheers,
Florent
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/8420#comment:11>
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.
