Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review Request : J. Stachelek #11

Closed
wants to merge 28 commits into from
Closed

Conversation

jsta
Copy link
Member

@jsta jsta commented Nov 3, 2015

AUTHOR

Dear @ReScience/editors,

I request a review for the replication of the following paper:

  • Least-cost modelling on irregular landscape graphs, T.R. Etherington. Landscape
    ecology, 27.7, 2012.

I believe the original results have been faithfully replicated
as explained in the accompanying article.

The repository lives at https://github.com/jsta/ReScience-submission/tree/STACHELEK

git clone https://github.com/jsta/ReScience-submission.git
cd ReScience-submission
git checkout STACHELEK
Rscript -e 'devtools::install_github("jsta/irlgraph", dependencies = TRUE)'
make all

EDITOR

  • Editor acknowledgment
  • Review 1 started (@dmcglinn) Nov 17, 2015
  • Review 2 started (@tpoisot) Nov 18, 2015
  • Review 1 decision [accept/reject] Feb 22, 2016
  • Review 2 decision [accept/reject] Feb 22, 2016
  • Editor decision [accept/reject] March 2, 2016

@jsta
Copy link
Member Author

jsta commented Nov 3, 2015

AUTHOR

My quick search for possible reviewers on Github turned up @Martin-Jung

@rougier
Copy link
Member

rougier commented Nov 3, 2015

EDITOR-IN-CHIEF

Thank you very much for the submission (as well as the suggested name of the reviewer).
An editor should soon acknowledge the submission to start the review. Don't hesitate to remind us if it was not the case in the next few days.

@jsta
Copy link
Member Author

jsta commented Nov 6, 2015

AUTHOR

Another potential reviewer might be @whalend

@FedericoV
Copy link

EXTERN

A colleague of mine, Duccio Rocchini (http://gis.cri.fmach.it/rocchini/) is quite experience in Python programming and geography and said he'd like to review. @rougier is it ok to invite him as a reviewer for this?

@rougier
Copy link
Member

rougier commented Nov 6, 2015

EDITOR-IN-CHIEF

Great, I will invite both to register to the board. In the meantime, we found an editor that should soon take over this review. @FedericoV Do you have the GitHub name for Duccio ?

@karthik
Copy link
Member

karthik commented Nov 9, 2015

EDITOR

Hi all,
I just wanted to chime in and say that I'll take over as editor of this submission. I'll follow up with reviewer recommendations shortly.

@karthik
Copy link
Member

karthik commented Nov 16, 2015

EDITOR

Just updating the thread that we'll finalize reviewers in a day or two. Sorry for the delay.

@karthik
Copy link
Member

karthik commented Nov 17, 2015

EDITOR

Reviewer 1, Dan McGlinn @dmcglinn has accepted.

@tpoisot
Copy link

tpoisot commented Nov 18, 2015

REVIEWER 2

In Makefile, the update_code target is rsync'ing local files (from /home/jose/...).

@tpoisot
Copy link

tpoisot commented Nov 18, 2015

REVIEWER 2

code/README.md still has a placeholder text.

@jsta
Copy link
Member Author

jsta commented Nov 18, 2015

AUTHOR

Thanks @tpoisot. Both of the issues arise because of my uncertainty with how to integrate my existing R package into the submission repo. I left the call to rsync in Makefile for my own benefit (note it is not called by the all target) but this could be removed or replaced by git clone or curl or something else.

I left the code/README.md file in the repo purely because it appears in the code repo of the first article. They place all of their code files at top level of the code folder whereas I follow the R package subfolder conventions. I would prefer simply to remove code/README.md in favor of code/irlgraph/README.md but I defer to the @ReScience/editors.

This also brings up the issue of devtools::install_github() versus R CMD INSTALL. Should I be treating the code associated with the article as static (R CMD INSTALL) or dynamic (devtools::install_github())?

@tpoisot
Copy link

tpoisot commented Nov 18, 2015

REVIEWER 2

For the last point -- I think you can use install_github to point to a particular commit using its hash, so you would keep the benefit of using this but still treat the code as essentially static.

@jsta
Copy link
Member Author

jsta commented Jan 15, 2016

AUTHOR

@karthik @tpoisot @dmcglinn I implemented a Docker pdf build in c03fe28

@karthik
Copy link
Member

karthik commented Jan 18, 2016

EDITOR

@dmcglinn @tpoisot Can you two do a pull from the current PR and build? It works fine for me. If you are also able to replicate, we can close this up before the end of January. Thanks very much.

PS: It finishes in under a minute.

@tpoisot
Copy link

tpoisot commented Feb 1, 2016

REVIEWER 2

Sorry for taking my sweet little time about testing.

Issue 1

Here is what I got:

▶ ReScience-submission on STACHELEK is clean $ make all
Rscript -e 'rmarkdown::render("code/irlgraph/vignettes/irlgraph.Rmd")'
Error in loadNamespace(name) : 
  aucun package nommé ‘rmarkdown’ n'est trouvé
Calls: :: ... tryCatch -> tryCatchList -> tryCatchOne -> <Anonymous>
Exécution arrêtée
Makefile:6 : la recette pour la cible « code/irlgraph/vignettes/irlgraph.pdf » a échouée
make: *** [code/irlgraph/vignettes/irlgraph.pdf] Erreur 1

I solved this by installing rmarkdown.

Issue 2

Here is the relevant line

pandoc --standalone --filter /home/tpoisot/.cabal/bin/pandoc-crossref --template=rescience-template.tex --latex-engine=xelatex --biblatex --bibliography=article.bib -M "crossrefYaml=crossref.yaml" --output article.tex article.md
pandoc: Filter /home/tpoisot/.cabal/bin/pandoc-crossref not found

On my system, the filter is called pandoc-citeproc, and pandoc-citeproc is in the path (and is at /usr/bin/pandoc-citeproc).

This one can be solved with the right variables passed to the different makefiles.

@jsta
Copy link
Member Author

jsta commented Feb 1, 2016

AUTHOR

@tpoisot
Have you seen the build instructions in README.md? They include installation of rmarkdown.

Are you able to build the article in the master branch (not STACHELEK)? The master branch article/README.md says to use pandoc-crossref not pandoc-citeproc...
I hope this is not related to the version problem discussed at #13

@jsta
Copy link
Member Author

jsta commented Feb 12, 2016

AUTHOR

@tpoisot @dmcglinn: According to @rougier in #13, you do not need to actually compile the pdf as reviewers. In order to build only the figures substitute the make all line in README.md with make panel-resize_images. The figure images can then be found in the article folder.

Also, you would not need the Haskell/cabal/pandoc-crossref dependencies.

@tpoisot
Copy link

tpoisot commented Feb 12, 2016

REVIEWER 2
@karthik Working, no more comments/requests.
:shipit:

@rougier
Copy link
Member

rougier commented Feb 13, 2016

EDITOR-IN-CHIEF

@karthik @tpoisot @dmcglinn Yes, the review is really about whether the original has been replicatedd or not and whether the new code can be ran easily. The actual buidling of the PDF is not necessary (it will be built once for the journal). The point of the review is really to check if the article explains why the author think results have been replicated and if the code is "ok" (comments, LICENSE, requirements) such that anybody reading the article can re-use th code.

@jsta Maybe you can remove the top Makefile (and change the README accordingly) to avoi the confusion.

Sorry if the instructions (http://rescience.github.io/edit/) are not clear enough. PR welcome !

@dmcglinn
Copy link

REVIEWER 1

@karthik this is working for me too. I also apologize for the long delays. No more comments.

On Feb 13, 2016, at 1:10 AM, Nicolas P. Rougier [email protected] wrote:

EDITOR-IN-CHIEF

@karthik @tpoisot @dmcglinn Yes, the review is really about whether the original has been replicatedd or not and whether the new code can be ran easily. The actual buidling of the PDF is not necessary (it will be built once for the journal). The point of the review is really to check if the article explains why the author think results have been replicated and if the code is "ok" (comments, LICENSE, requirements) such that anybody reading the article can re-use th code.

@jsta Maybe you can remove the top Makefile (and change the README accordingly) to avoi the confusion.

Sorry if the instructions (http://rescience.github.io/edit/) are not clear enough. PR welcome !


Reply to this email directly or view it on GitHub.

@karthik
Copy link
Member

karthik commented Feb 14, 2016

EDITOR

Thank you @tpoisot and @dmcglinn
I'll look this over shortly and update the thread.

@karthik
Copy link
Member

karthik commented Mar 3, 2016

EDITOR

@jsta Apologies for all the delays. But I have also reviewed the submission and the original landscape ecology paper by Etherington and am happy that the original code and figures have been reproduced. I'm happy to accept the submission.

@jsta
Copy link
Member Author

jsta commented Mar 4, 2016

AUTHOR

Thanks @karthik , please let me know if I can do anything else to prep the PR for import.

@rougier
Copy link
Member

rougier commented Mar 4, 2016

EDITOR-IN-CHIEF

@jsta Until actual import (soon), please do not commit to the repo.
@karthik I can take care of the final publication if you want.

@rougier
Copy link
Member

rougier commented Mar 7, 2016

EDITOR-IN-CHIEF

@karthik I will take care of the publication.
@jsta You need to have all the output images either in your code repository (or in the article repository) such that I can build the PDF without running the code. Does that make sense ?

@jsta
Copy link
Member Author

jsta commented Mar 7, 2016

@rougier I have added many images to the repo. You should be able to build
the PDF now.
On Mar 7, 2016 5:24 AM, "Nicolas P. Rougier" [email protected]
wrote:

@karthik https://github.com/karthik I will make the publication.
@jsta https://github.com/jsta You need to have all the output images
either in your code repository (or in the article repository) such that I
can build the PDF without running the code. Does that make sense ?


Reply to this email directly or view it on GitHub
#11 (comment)
.

@rougier
Copy link
Member

rougier commented Mar 7, 2016

@jsta Thanks.

@rougier
Copy link
Member

rougier commented Mar 7, 2016

@jsta Could you also give 2/3 keywords ? (We need to add a keyword fields in the template)

@rougier
Copy link
Member

rougier commented Mar 7, 2016

This submission has been accepted for publication, and has been published and will soon appear at https://github.com/ReScience/ReScience/wiki/Current-Issue

DOI

@rougier rougier closed this Mar 7, 2016
@ReScience ReScience locked and limited conversation to collaborators Mar 7, 2016
@karthik
Copy link
Member

karthik commented Mar 8, 2016

Thanks @rougier!

@rougier rougier added the R label Jul 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants