emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: 5x5 Arithmetic solver


From: Vinicius Jose Latorre
Subject: Re: 5x5 Arithmetic solver
Date: Sat, 21 May 2011 11:36:52 -0300
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110414 Lightning/1.0b2 Thunderbird/3.1.10

Hi Vincent,

Could I suggest the attached diff?

Vinicius


On 05/21/2011 04:15 AM, Vincent Belaïche wrote:
Salut Stéfan,

Toujours aussi à cheval sur les conventions de codage !

Please find herein attached a contribution to the 5x5 game. This is an
arithmetic solver based on a matrix inversion in a (Z/2Z)^25 vector
space.
Thanks.  A few comments:
- avoid using a tarball and just attach the diff as-is,
  makes it a lot easier to review.
noted

- why 5x5-local-variables?
That is used in 5x5-mode to imply that all listed variable are made
local to that very 5x5 buffer. if after `M-x 5x5' you do a `M-x
rename-uniquely' and then `M-x 5x5' again then you have have two
independent games.

- explain the changes in the 5x5 function.
I had to change slightly the order of operations because setting the
mode has to be done before any buffer local 5x5 variable is touched, as
precisely those variables are made local by setting the mode

- many of your lines have trailing whitespace.  I generally don't care
  much, about it, but some people do, and it's usually preferable to
  avoid them.  M-x picture-mode C-c C-c gets rid of them for you (as
  a side-effect).
Done

- try to keep the first line of docstrings as a self-sufficient sentence
  (because M-x apropos only shows the first line).


- stay within 80 columns.
Do you mean that we are still in the 80ies ;-P ?

Ok, done.

- your code is not properly indented (e.g. the `grid' argument in
  5x5-grid-to-vec).
Done

- Please capitalize your comments and terminate them with a "." or some
  other appropriate punctuation.
Some comments by a variable name, so they are not capitalized.

Some comments are equations like this:

           ;; B:= targetv
           ;; A:= transferm
           ;; P:= base-change
           ;; P^-1 := inv-base-change
           ;; X := solution

           ;; B = A * X
           ;; P^-1 * B = P^-1 * A * P * P^-1 * X
           ;; CX = P^-1 * X
           ;; CA = P^-1 * A * P
           ;; CB = P^-1 * B
           ;; CB = CA * CX
           ;; CX = CA^-1 * CB
           ;; X = P * CX

I don't think that this is common practice to use a punctuation at the
end of an equation.

Some other comment is commented out code like this:

           ;;(ctransferm-1-2 (calcFunc-mcol ctransferm-1-: col-2))

This code is not there because I don't need that variable, but I found
useful to show how it would be defined.

For all the remaining comments I have done it

- 5x5-solve-suggest should have a docstring.
Done

- try C-u checkdoc-current-buffer.
I get

checkdoc-continue: Too many occurrences of \[function].  Use \{keymap}
instead

Because 5x5 is used many times as if it was


- we need a ChangeLog entry.
Here it is:

-----------------------------------------------------------------------
2011-05-21  Vincent Belaïche<address@hidden>

        * play/5x5.el: Add an arithmetic solver to suggest positions to
        click on.
-----------------------------------------------------------------------

- I don't understand the "solve step" message (e.g. it said 23 every
  time, even though I followed its suggestions and finished in 12 moves).


Ask it to Jay, this message is output by Calc, not by 5x5. 23 is due
to this that you have to invert a 23x23 matrix. Altough the 5x5 transfer
matrix is 25x25, its rank is only 23, so I extract some submatrix to
compute the solution.

        Stefan
   Vincent.


Attachment: 5x5.el.diff-suggestion
Description: Text document


reply via email to

[Prev in Thread] Current Thread [Next in Thread]