[RkWard-devel] Grid, Stepfun and final (nearly) form of the clt plugin.

[Thomas Friedrichsmeier]

Hi,

some comments (have not looked at everything in detail, yet):

On Thursday 01 March 2007 21:28, Prasenjit Kapat wrote:
> 1. plot_stepfun_options: This plugin provides the the graphical options to
> plot.stepfun(). As such this is to be used an embeded plugin. We can write
> a new plugin, plot_stepfun (under plots?), with a varslot element and
> embeding plot_stepfun_options. This will complete the plot.stepfun()
> function.

Why didn't you place the colors for 0 and 1 lines to the plot_stepfun_options? 
Formatting concerns? Any chance of working around those?

> 2. A grid plugin under (X11device) View > Draw grid. A small one but covers
> all the arguments to grid(). The drawback (when compared to the grid on/off
> option of matlab) is that, once the grid is drawn it cannot be erased
> (off?). But thats the nature of the R function grid(). One solution is to
> include this in the plot_options plugin with a checkbox to enable/disable
> the grid.

This one looks good to me. Simple and useful. Maybe place it under a (new) 
menu "Edit" instead of View? After all, it really modifies the plot, instead 
of just the view.

Indeed, it would probably be nice to have this in plot_options. I guess we 
could use the calculate() section for this and potential other extensions 
(such as rug), which require separate commands (not just options to plot).

Also having the grid plugin as a standalone in the x11 device context is nice, 
I think, and it should do no harm to keep it there, as well.

This may be one of the few plugins which can easily be modified to work both 
embedded and standalone. For this, you'd add an <external id="context" />. If 
this is set to "x11", the dev.set() line is needed, else it is not. This 
should probably suffice. Now, where would this sort of plugin be placed in 
the directory structure...?

> Based on how "preview" works, as soon as this (grid) checkbox is 
> toggled, the whole graphics will be redrawn again, therby giving a "feel"
> of grid on/off. Why is it a "feel"? Because for simulated data plots (like
> distribution/clt plugins) every time the graphics is redrawn, it is for a
> whole new set of random numbers. But again, this is the situation with any
> GUI element not just the grid checkbox (if added).

True, it's a bit awkward. On the other hand, outside the CLT plugins, the 
plotted data will typically be static instead of random, so I guess it's not 
too much to worry about.

> 3. The binomial CLT: Apart from the "rug" options, I think the plugin is
> ready to go. I am not planing to add QQplot, it is becoming an overkill for
> a simple visualisaton.

OK. A few comments, while I'm looking at it:
1) I believe the $mean and $var are only needed in the if ($scalenorm) case, 
and hence should be moved inside the if. Or maybe you can avoid these lines 
altogether, as they are really just two plain numbers (however, then it's not 
as easy to see, what those numbers signify, so not sure on this).
2) Maybe a few comments would be a nice extra, and help understand the code. 
Such as:
# generate samples
# normalize
# adjust y range
Not sure which ones, and how many, but it might make the code more legible.
3) Move the $hist <- and $ylim <- outside the rk.graph.on()/off() block, as 
these are still preparation, not plotting.
4) I suggest to use a single try statement around all the plot functions. The 
main point of try in this case is to make sure we reach the rk.graph.off(), 
after we have made it to rk.graph.on().
5) Indentation (my favorite nitpick):
- I usually try to keep the indentation of the PHP code itself as it would be 
without the intervening printout. In this case, this would mean replacing 
some statement like
<? }
if (something) { ?>
with
<?	}
	if (something) { ?>
There's single tab each, between "<?" and "}" and at the start of the second 
line.
- In the generated code, the "try( lines(..." is indented by two spaces, but 
is not acutally on a different level of nesting than the lines below and 
above.

> 4. As Thomas was saying, rug() is a generic plot feature, which can be
> treated in a fashion similar to grid(). So, if everyone feel the same way,
> I will remove "Rug options" from clt and add a new embedable plugin which
> can be used anywhere (may be plot_options or may be (x11device) VIew > Add
> Rug or somewhere once we reach a working solution for par() options).

I guess it would be similar to "grid". Note that for a rug-options plugin, it 
may make sense to support both top/bottom and left/right rugs.

> Once the rug issue for CLT is settled, I am planning to roll out the clt
> plugin for all the distributions, and then be done with CLT. Now, that
> means a lot of new files (19*3) under distributions dir. Is that fine, or
> should we create a new dir (may be sub-dir under distribution)?

I guess a sub-dir may make sense (distributions is a pretty crowded dir, 
already), but either way is fine with me.

Before rolling out the remaining CLT plugins, I suggest to get extensive 
review of the binomial CLT, first (as mentioned in the other mail).

> Lastly, the "addRugtoplot" checkbox on binomial_clt.xml is behaving funny.
> I have connected all the rug related options to be enabled based on
> addRugtoplot.state. But it does not work the first time the plugin is
> started (when the rug checkbox is not checked but still all the options is
> enabled). I will look into it again, today.

That's strange, indeed. Interestingly, if you set ECDF to be the default mode, 
it works correctly. Almost certainly a bug on the C++ side. I'll look into it 
later (remind me, if I don't).

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/20070301/e89432c4/attachment.sig>