[RkWard-devel] RKWard 0.4.5pre1, 0.4.5 release process

[Thomas Friedrichsmeier]

Hi!

On Thursday 11 January 2007 19:43, I. Soumpasis wrote:
> Sending
> plots_cor_graph.tar.gz<?&ik=90971e2fbb&view=att&th=110126b9da3bad04&attid=0
>.1>containing the two files (xml and php) right now.

A number of nitpicks (but it does look pretty good, overall):
1) You put all the R code inside printout (). The point of having different 
sections is to provide a little common structure. In this case, the 
definition of the helper functions should go into preprocess (), the call to 
rm() should go to cleanup ().

2) Indentation: The next line after an opening brace ('{') should always be 
indented by one Tab (until the closing '}'). This makes reading the code 
structure much easier. Also, maybe you could replace the spaces that are used 
to indent the panel.cor()-block right now with tabs, as well.

3) It would probably be useful to have the "method", and "use" options to cor 
and cor.test as well. You can copy the relevant sections from the correlation 
matrix plugin. One way to make this work, would be to extend the panel.cor 
function with those two paramenters, then modify the call to pairs to:

pairs(x, lower.panel=panel.smooth, upper.panel=function (x, y) panel.cor (x, 
y, use, method))

4) The "digits" and "prefix" parameters to panel.cor are effectively unused, 
currently. I don't really understand, what the prefix parameter is for, but 
maybe you could also add a GUI option for the digits parameter (that, or 
remove it to keep the code simple).

5) You wrap up everything in a function rk.temp.cor.graph (). I'm not entirely 
sure, whether I like or dislike this (so far we did not use this). Anybody 
have an opinion already?

6) It's usually a good idea to wrap everything between rk.graph.on () and 
rk.graph.off () in a try () statement, i.e.:
rk.graph.on ()
try ({
	rk.temp.cor.graph (rk.temp.x)
})
rk.graph.off ()
This makes sure that the graphing device gets shut down in case there was an 
error during the plot. Which reminds me, that I intended to add this 
everywhere else...

7) The idea of scaling the text the r is printed in according to the value of 
r is definitely interesting, but I think it should be possible to turn it 
off.

All this may seem like a lot of criticisms, but mostly it's small or optional 
stuff, so I hope you don't feel discouraged. The most important thing is that 
it works and is useful, and your code works nicely indeed, and is a pretty 
nifty feature! If you need more details/comments, just ask.

Regards
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/rkward-devel/attachments/20070111/d7f5dc77/attachment.sig>