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

[Thomas Friedrichsmeier]

On Friday 12 January 2007 09:54, I. Soumpasis wrote:
> 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.
>
> You 're right. Done.

Sorry, I wasn't precise: The indentation I was worried about is the 
indentation of the generated code, not the indentation as visible in the .php 
file. I.e. inside the preprocess () function, you'd unindent everything by 
two tabs, inside the printout (), and cleanup () functions by one tab. Then, 
the generated code should look "clean". (See the new section on that in the 
plugin documentation: 
http://rkward.sourceforge.net/documents/devel/plugins/phptemplate.html#policyformatting)

> 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))
>
> I have done it with a different method, don't know if it is ok. I applied
> use, method to the cor and cor.test functions. Tested the method and works.
> Is this ok or should I try the method proposed?

I think there is one advantage to the method I proposed: I would make the 
panel.cor () function truly reusable as is. All parameters would be set at 
only one single point: The function call itself. The user would not have to 
modify the function body itself to re-run e.g. with method "spearman" instead 
of "pearson". I think in this case this method would not introduce too much 
additional complexity, and be useful at the same time, so I think it's 
better.

> 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).
>
> I gave an option to the user to select precision (1,2,or 3 digits - Sould
> we do something else here?). Removed prefix.

It's probably nicer to use a <spinbox type="integer" min="0" max="5" 
initial="2"> instead of a <radio> for this task. After all, it's a numerical 
selection.

> 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?
>
>   There is not a function that can do this to my knowledge so I made this
> one using the example. This is the way I have implemented it in R and just
> pasted here. One good thing is that you have to delete only this one
> function stored in R (and your data). One other that someone could pass
> scripts or functions already written in a very easy way. I don't know.
> Should I change that?

The alternative would be to rename "panel.cor" to "rk.temp.panel.cor", then 
change
	rk.temp.cor.graph (rk.temp.x)
to:
	pairs (x, lower.panel=panel.smooth, upper.panel=rk.temp.panel.cor)
(or considering 3) to):
	pairs (x, lower.panel=panel.smooth, upper.panel=function (x, y) 
rk.temp.panel.cor (x, y, digits=<? getRK ("digits"); ?>, use="<? getRK 
("use"); ?>", method="<? getRK ("method"); ?>"))

However, since I'm not quite sure myself, just leave everything inside the 
rk.temp.cor.graph () function for now, until I've made up my mind.

> 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.
>
> Done.

Since this is a yes/no question, a <checkbox> would be better suited than a 
<radio>. Considering 3), you might want to add scale as a parameter to 
panel.cor, too. Also, for this type of selection, you might want to set the 
option value to "TRUE" or "FALSE", instead of "yes" or "no". Then you can 
simplify a statement like:

	if("<? getRK ("scale"); ?>"=="yes") [...]
to
	if(<? getRK ("scale"); ?>) [...]

> 1. I added a note with what the asterisks mean.

Very useful, indeed.

> 2. I  believe that the plugin should be placed under the same menu with
> correlation matrix, I mean something like this.
>
> Analysis -- Correlation -- Correlation matrix
>
>                                   -- Correlation graph

Not sure, yet. Anybody else have an opinion?

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/20070112/e0138e52/attachment.sig>