search

ProjectMOSAIC / mosaic

Project MOSAIC R package
http://mosaic-web.org/
93 stars 26 forks source link

message => cat, in statTally? #732

Closed homerhanumat closed 5 years ago

homerhanumat commented 5 years ago

Some students this week were writing up a homework assignment in R Markdown and had globally set the chunk option message = FALSE (as they use ggplot and did not want its frequent messages showing in the compiled output). Therefore their calls to statTally() showed only the table of percentiles (and of course the histogram). Perhaps when statTally() is revised the console text should be produced with cat() rather than message()?

dtkaplan commented 5 years ago

Hi Homer. The reason to use message() is, I think, that it can be controlled on or off. As a rule, returning values is better than side effects. But Randy might think differently.

homerhanumat commented 5 years ago

statTally() uses print() to display the table of percentiles, which is also a side effect and cannot be controlled with chunk options. To get control, add a verbose parameter (default TRUE, maybe) and a graph parameter. This would match the behavior of other mosaic functions more closely, I think.

It seems to me that the purpose of message() (like stop() and warning()) is to provide information to R-coders, not to end-users (e.g., someone reading a report produced by R Markdown).

rpruim commented 5 years ago

I'm still mulling over what I think is best to do here.

rpruim commented 5 years ago

message(), warning() and stop() are definitely intended for users. The latter two alert the user to (potential) problems with their code, and hopefully provide a pointer toward how to fix it. message() is similar, but the use case is a bit less well defined, I think. (cf all those messages from ggplot2 -- and note that gf_histogram() doesn't nag about bins every time :-)

Our situation is tricky because our functions does three things: (1) it produces and prints a plot, (2) it prints some information, (3) it returns something. Because of this the standard "return and object that has a custom print method" doesn't work well here.

I agree that having the table of percentiles respond differently from the rest of the output is not desirable, and that should be fixable.

rpruim commented 5 years ago

all messages now

rpruim commented 5 years ago

It would be relatively easy to make it all cat() instead.

rpruim commented 5 years ago

Changed to cat() and implemented quiet = argument to turn off the text printout, if desired. This seems to be a reasonable solution (and to fit what @homerhanumat had in mind).