berndbischl / BBmisc

Other
20 stars 7 forks source link

makeProgressBar() outputs to stdout - ideally stderr #31

Closed HenrikBengtsson closed 10 years ago

HenrikBengtsson commented 10 years ago

BBmisc::makeProgressBar() outputs to the standard output. The preferred is to output to standard error for such messages, cf. message(). The problem with sending this to stdout is that report generators such as Sweave, knitr, R.rsp etc. echo stdout to the generated report. This means that the progress bar of BBmisc clutter up the report (and won't be displayed as "progress" to the user while actually running).

I would consider this a major issue or even a critical one, since it renders BatchJobs progress bars useless and unusable for report generators.

**EXAMPLE:

> library("BBmisc")
> options(BBmisc.ProgressBar.width=50)
> bar <- makeProgressBar(max=5, label="test-bar")
> bar$inc(1)
test-bar |++++                 |  20% (00:00:00)>
> bfr <- capture.output(bar$inc(1))
> bfr
[1] "\rtest-bar |++++++++             |  40% (00:00:04)"

capture.output() captures stdout - not stderr.

> sessionInfo()
R version 3.1.0 Patched (2014-04-17 r65403)
Platform: x86_64-w64-mingw32/x64 (64-bit)

locale:
[1] LC_COLLATE=English_United States.1252
[2] LC_CTYPE=English_United States.1252
[3] LC_MONETARY=English_United States.1252
[4] LC_NUMERIC=C
[5] LC_TIME=English_United States.1252

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base

other attached packages:
[1] BBmisc_1.5

loaded via a namespace (and not attached):
[1] tools_3.1.0
berndbischl commented 10 years ago

Henrik,

thx for the hint. I have currently finished a new release. I could still include a change for this.

But you do know that it is a documented feature to simply turn the progress bar off, if it clutters something?

berndbischl commented 10 years ago

Do you see a problem if I simply substitute the calls to cat in the progressbar to cat(..., file = stderr())?

HenrikBengtsson commented 10 years ago

Thanks for the quick reply. I'd strongly propose/favor that this would go in any updates you publish on CRAN.

I don't see an issue by using cat(..., file=stderr()). Don't forget to update kill() in addition to draw(). Not sure if there are other places. If you want to be conservative/backward compatible, you could add an option for stdout or stderr, but honestly I don't see why someone would want stdout.

Yes, while you replied I was looking for options how to disable the progress bar. I was over at BatchJobs looking at its docs, but then I found it in BBmisc. In case someone else reads this:

library("BBmisc")

# Disable progress bar (must be done before calling makeProgressBar())
options(BBmisc.ProgressBar.style="off")
bar <- makeProgressBar(max=5, label="test-bar")
bar$inc(1)

# Enable progress bar (must be done before calling makeProgressBar())
options(BBmisc.ProgressBar.style="text")
bar <- makeProgressBar(max=5, label="test-bar")
bar$inc(1)
berndbischl commented 10 years ago

1) I thought about adding an option for the streams, but you are probably right. I would make stderr the default anyway.

2) I know all the places where to change this for the bar, this are not many, the code is simply in one (small) R file. I wonder whether there are other problems in BatchJobs though. But I guess we always use "message" for all other prints? So we are OK?

I will scan BBmisc and BatchJobs with ack for other uses of "cat" to make sure.

berndbischl commented 10 years ago

And I will add how to turn the bar off to the FAQ of BJ.

HenrikBengtsson commented 10 years ago

Awesome!

berndbischl commented 10 years ago

I will post tomorrow, when its done.

For now: Have a happy easter.

HenrikBengtsson commented 10 years ago

Happy Easter to you too.

berndbischl commented 10 years ago

1) I do see a couple of places where we use cat / catf in BJ. But none seem to be relevant here, it is stuff like print.class, job output on slaves and so on. So I think we are fine for now.

berndbischl commented 10 years ago

2) I have pushed the changes now. Lest we later discover that we forgot something in the discussion, I have made the stream selectable (default = stderr) and updated the docs.

@HenrikBengtsson: Do you want to check?

berndbischl commented 10 years ago

3) I have added a note to the FAQ. Nearly every link in the FAQ was dead, I fixed that too....

berndbischl commented 10 years ago

@mllg Before I close I would like to get a 3rd opinion. Michel, do you think it is OK now? Anything to add?

mllg commented 10 years ago

Looks good. Closed.