SimonGoring / neotoma_paper

Repository for the neotoma package paper.
MIT License
3 stars 2 forks source link

Paper is getting out of whack, technically #14

Closed gavinsimpson closed 9 years ago

gavinsimpson commented 10 years ago

We need to give the paper a clean up as various changes are breaking functionality or introducing issues. Two that I am currently aware of are:

  1. There are two code chunks that change the way knitr operates.

    • the one starting on Line 15, which is the main options chunk
    • the one starting on Line 152, which introduces some of @karthik's (KR?) defaults.

      We need to reconcile these into a single settings chunk at the start of the paper.

  2. Something has broken the Makefile, and I suspect it is that somewhere along the line library("knitr") has gotten removed from the paper. Now, it shouldn't be there as you don't need it to follow the neotoma examples but only to build the .Rmd. However, the Makefile breaks because it doesn't load knitr and the paper was written (currently) assuming knitr was loaded and attached to the search path.

    We could add library("knitr") to the Makefile Rscript invocations, or we could, I presume change kable() into knitr::kable() through the paper. The former is most explicit.

A third issue is how we build the paper. It seems that we have two options currently:

A disadvantage of the latter is that it doesn't generate intermediate files (at the moment --- though I suppose we can get it to do that). It is important to have a rendered .md and associated images in figures/ if we want people to take our .Rmd source and work with it because rmarkdown is not yet available on CRAN and users, especially those on Windows, will have to jump through extra hoops to work with these sources.

@karthik or @sckott could you comment on how you envisage these papers being built as I understand you are trialling some pan-rOpensci approaches to this which are in use here?

karthik commented 10 years ago

Item 1 is resolved in the latest merge. so is 2 since the library is loaded explicitly somewhere in the middle now and I have been using Make to rebuild the paper of late.

@gavinsimpson We're not going for any fancy approach here. For now I'm just using the Makefile to build all targets (word and pdf) and for the osx and linux people this should work just fine. I know nothing what make options are available for windows.

gavinsimpson commented 10 years ago

@karthik thanks for the update.

Re 2., the paper is now loading knitr but this is wrong. The reader doesn't need knitr loaded to follow the examples or use neotoma. I'm going to rip out that line in a PR shortly. The upshot is that the Makefile will need to be updated to work correctly, or we adjust the paper to use say knitr::kable(). As all of this is about getting knitr to render the Rmd to some other formats and the reader is not intended to use knitr themselves, we should do all of this out of their sight.

So I intend to rip out library("knitr") and prefix all calls to knitr functions with knitr::.

Any objections to this?

karthik commented 10 years ago

Re 2., the paper is now loading knitr but this is wrong.

Agreed. I didn't add it there.

gavinsimpson commented 10 years ago

@karthik Yup, I know. Hence my writing this Issue in the first place...

karthik commented 10 years ago

and no objections. But I'm a little confused. In the makefile, I explicitly load rmarkdown which makes all the knitr functions available.

Rscript -e "library(rmarkdown); render('Neotoma_paper.Rmd', 'pdf_document')"

So why deal with knitr:: in the text?

We're not using kable anymore, it's been replaced with pander::pandoc.table.

gavinsimpson commented 10 years ago

rmarkdown imports the knitr namespace; it doesn't make the knitr functions available for use within the document being knitted, only internally for itself. We were calling two knitr functions:

  1. kable(), and
  2. opts_chunk()

Originally the paper only used the latter of these two and did so via a knitr:: prefix. Hence everything worked OK initially, although perhaps by accident rather than implicit design. Once kable() was used, the Makefile broke because it didn't load knitr.

To some extent this is moot now as things have changed again because we use knitr::opts_chunk() and have dropped the usage of kable(). However, the Makefile is still broken if you intend authors to be able to use knitr functions within the R instance you spawn as part of the operations of make.

gavinsimpson commented 10 years ago

@karthik The Makefile really needs to be

Rscript -e "library('rmarkdown'); library('knitr'); render('Neotoma_paper.Rmd', 'pdf_document')"

if you want knitr functions available for use within the document without knitr:: prefix.

SimonGoring commented 9 years ago

Hi @gavinsimpson and @karthik I assume this issue is closed now. Can you confirm?

karthik commented 9 years ago

Yeah, fine with me.

gavinsimpson commented 9 years ago

@SimonGoring Yep, between us we got all those niggling issues ironed out before the initial submission.