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]: fuse: An R package for ensemble Hydrological Modelling #52

Closed
16 tasks done
whedon opened this issue Aug 16, 2016 · 53 comments
Closed
16 tasks done

[REVIEW]: fuse: An R package for ensemble Hydrological Modelling #52

whedon opened this issue Aug 16, 2016 · 53 comments
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Aug 16, 2016

Submitting author: @cvitolo (Claudia Vitolo)
Repository: https://github.com/cvitolo/fuse
Version: v3.0.0
Editor: @arfon
Reviewer: @masalmon
Archive: 10.5281/zenodo.212822

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/392a55daada04a86f95eaa8da134a28d"><img src="http://joss.theoj.org/papers/392a55daada04a86f95eaa8da134a28d/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/392a55daada04a86f95eaa8da134a28d/status.svg)](http://joss.theoj.org/papers/392a55daada04a86f95eaa8da134a28d)

Reviewer questions

Conflict of interest

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v3.0.0)?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: Have any performance claims of the software been confirmed?

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

Paper PDF: 10.21105.joss.00052.pdf

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?
@whedon whedon added the review label Aug 16, 2016
@arfon
Copy link
Member

arfon commented Aug 16, 2016

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

  • Please work through the checklist at the start of this issue.
  • If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
  • Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

@pragyansmita
Copy link

library(devtools)
install_github("cvitolo/r_fuse", subdir = "fuse")
Downloading GitHub repo cvitolo/r_fuse@master
from URL https://api.github.com/repos/cvitolo/r_fuse/zipball/master
Error: Could not find build tools necessary to build fuse

I am using R 3.2.5 and have Rtools33 installed. I am still receiving the above error. Any idea how to move beyond to install fuse. Thanks!

@pragyansmita
Copy link

✋ I am reviewing this

@cvitolo
Copy link

cvitolo commented Aug 26, 2016

@pragyansmita I'm sorry you cannot install fuse. I work only with Linux machines and I cannot reproduce your problem. I use travis-ci to check that there are no problems with installation and tests. As you can see here the package passes all the tests and builds no problem on a linux machine.

@maelle
Copy link

maelle commented Aug 26, 2016

@cvitolo by the way on Travis you can also test on OSX by adding

os: 
 - linux
 - osx

after language: r at the top of your .travis.yml.

Then you can use Appveyor for continuous integration on Windows (there's a devtools::use_appveyor() function which automatically creates the Appveyor config file, and then you need to activate CI for the repository on Appveyor website ), quite similarly to what you did on Travis.

Furthermore on Travis and Appveyor there are options for testing your package with several R versions, I do it here and here.

Not sure if it'll help you see @pragyansmita's problem, but who knows :-)

@cvitolo
Copy link

cvitolo commented Aug 26, 2016

Many thanks @masalmon!

I just added the os: linux osx lines to .travis.yml. Building the job for osx takes ages, is that normal?

Also tried to configure appveyor but I keep getting this error 'Specify a project or solution file. The directory does not contain a project or solution file.'... not sure how to fix this.

@maelle
Copy link

maelle commented Aug 26, 2016

You're welcome!

I don't know for osx, maybe it's just because it's the first build?

I am sorry I can't help for Appveyor, I never had this issue. Did you need to do something specific on Travis because of the sub directory structure?

@cvitolo
Copy link

cvitolo commented Aug 26, 2016

I just did:

cd fuse

I tried the same in appveyor but that did not fix the problem

@maelle
Copy link

maelle commented Aug 26, 2016

@maelle
Copy link

maelle commented Aug 26, 2016

Although that's what you already did...

@jankatins
Copy link

jankatins commented Aug 26, 2016

Specify a project or solution file. The directory does not contain a project or solution file.

I thought this happens when appveyor doesn't see a appveyor.yml file or the file doesn't have a build section (or whatever the section is named which builds the project) -> per default it tries to build a MS Visual Studio solution / project file (like the default makefile for make based systems).

-> You have to specify how to build the project otherwise it tries the default which fails

@jkahn
Copy link

jkahn commented Aug 26, 2016

Apologies for intervening, but could these details be discussed in an issue on the submitted project rather than here in the review?

I'll file a ticket with @arfon to encourage this as a general directive to reviewers.

@arfon
Copy link
Member

arfon commented Aug 26, 2016

Apologies for intervening, but could these details be discussed in an issue on the submitted project rather than here in the review?

Good suggestion @jkahn. @masalmon @cvitolo @JanSchulz - would you mind taking this discussion over to the submission repo (https://github.com/cvitolo/r_fuse)?

@cvitolo
Copy link

cvitolo commented Sep 4, 2016

Thanks @arfon! I opened an issue on the r_fuse repo. We will continue the discussion there.

Is there anything else that needs to be done?

@cvitolo
Copy link

cvitolo commented Sep 5, 2016

@arfon I cloned the repository to fuse and moved the package to the root dir. That fixed all the problems with appveyor. Would it be possible to continue the review on the cloned repo? Alternatively, I need to rename the r_fuse repo.

@arfon
Copy link
Member

arfon commented Sep 5, 2016

@arfon I cloned the repository to fuse and moved the package to the root dir. That fixed all the problems with appveyor. Would it be possible to continue the review on the cloned repo?

@cvitolo - just to clarify. You would like to replace the repository associated with this submission from https://github.com/cvitolo/r_fuse to be https://github.com/cvitolo/fuse?

@cvitolo
Copy link

cvitolo commented Sep 6, 2016

@arfon Yes, please. Would that be possible? Thanks!

@arfon
Copy link
Member

arfon commented Sep 6, 2016

@arfon Yes, please. Would that be possible? Thanks!

Sure thing. The review is now associated with https://github.com/cvitolo/fuse. @pragyansmita the updated paper is 10.21105.joss.00052.pdf

@cvitolo
Copy link

cvitolo commented Sep 7, 2016

@pragyansmita I don't know what prevents the fuse package to install on your system. The package seems to build fine for linux and windows systems. I have also tested various r versions and get no errors, as you can see here. Do you mind to give it another try? Please let me know if you still have problems.

@arfon Many thanks for changing the repo link!

@cvitolo
Copy link

cvitolo commented Sep 14, 2016

@arfon @pragyansmita Just wanted to know whether there is anything I can do to proceed with the review?

@arfon
Copy link
Member

arfon commented Sep 14, 2016

@pragyansmita - are you ok to start moving forwards with your review?

@arfon arfon changed the title Submission: fuse: An R package for ensemble Hydrological Modelling [REVIEW]: fuse: An R package for ensemble Hydrological Modelling Sep 20, 2016
@arfon
Copy link
Member

arfon commented Sep 20, 2016

@whedon commands

@whedon
Copy link
Author

whedon commented Sep 20, 2016

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the reviewer of this submission
@whedon assign @username as reviewer

# List the GitHub usernames of the JOSS editors
@whedon list editors

# List of JOSS reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Open the review issue
@whedon start review

🚧 Important 🚧

This is all quite new. Please make sure you check the top of the issue after running a @whedon command (you might also need to refresh the page to see the issue update).

@arfon
Copy link
Member

arfon commented Sep 20, 2016

@whedon assign @arfon as editor

@whedon
Copy link
Author

whedon commented Sep 20, 2016

OK, the editor is @arfon

@arfon
Copy link
Member

arfon commented Sep 20, 2016

@whedon assign @pragyansmita as reviewer

@whedon
Copy link
Author

whedon commented Sep 20, 2016

OK, the reviewer is @pragyansmita

@maelle
Copy link

maelle commented Dec 17, 2016

This is a cool package, congrats on making the initial Fortran code in a package and available to R users!

Reviewer questions

Conflict of interest

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v3.0.0)?

No, the development version is 3.2 and the release version is 3.1.

Functionality

  • [ x] Installation: Does installation proceed as outlined in the documentation?
  • [ x] Functionality: Have the functional claims of the software been confirmed?
  • [ x] Performance: Have any performance claims of the software been confirmed?

Documentation

  • [x ] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

Yes. Actually the part "The fuse framework takes as input rainfall and potential evapotranspiration time series (areal averages over the river catchment area) and returns a simulated time series of river discharges. It can be used to understand the variability of expected hydrological responses based on model structures. " of the paper could even be at the beginning of the README & vignette (or in the help page for the package.) too.

  • [x ] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • [ x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • [ x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?

I have opened quite a few issues with suggestions for the documentation, but they are nearly all only suggestions.

  • [x ] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?

The test coverage could be increased a bit though.

  • [x ] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

The only thing that's not stated at the bottom of the README is whether code contributions are welcome.

Software paper

Paper PDF: 10.21105.joss.00052.pdf

  • [ x] Authors: Does the paper.md file include a list of authors with their affiliations?
  • [ x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [ x] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

@cvitolo
Copy link

cvitolo commented Dec 18, 2016

@masalmon Many thanks for your thourough review, your suggestions greaty improved the consistency of the package.

I'm planning to update the release when the review is finished.

I have added the description of the package (as in the paper) to the README and vignette as you suggested.

I have added many more tests and addressed all the issues.

I have added info on related packages and future developments to the paper.

I have added a statement in the README saying code contributions are welcome.

@maelle
Copy link

maelle commented Dec 18, 2016

Looks good to me, thanks for your work @cvitolo ! 👏

@arfon I'd recommend accepting the paper now 😀

@arfon
Copy link
Member

arfon commented Dec 20, 2016

Thanks @masalmon!

@cvitolo - at this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

@cvitolo
Copy link

cvitolo commented Dec 20, 2016

@arfon I just made a new release (v3.2), here is the new doi: http://doi.org/10.5281/zenodo.212822

@arfon
Copy link
Member

arfon commented Dec 21, 2016

@whedon set 10.5281/zenodo.212822 as archive

@whedon
Copy link
Author

whedon commented Dec 21, 2016

OK. 10.5281/zenodo.212822 is the archive.

@arfon
Copy link
Member

arfon commented Dec 21, 2016

@masalmon @pragyansmita - many thanks for the review here ✨

@cvitolo - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00052 ⚡️ 🚀 💥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

7 participants