search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: Isoreader: An R package to read stable isotope data files for reproducible research #2878

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @sebkopf (Sebastian Kopf) Repository: https://github.com/isoverse/isoreader Version: v1.3.0 Editor: @mikldk Reviewers: @kdorheim, @r-barnes Archive: 10.5281/zenodo.4724083

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@kdorheim and @r-barnes, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @mikldk know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @kdorheim

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @r-barnes

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @kdorheim it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1111/gbi.12312 is OK
- 10.1002/lom3.10300 is OK
- 10.1111/gbi.12386 is OK
- 10.1144/SP507-2020-85 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mikldk commented 4 years ago

@kdorheim: Thanks for agreeing to review. Please carry out your review in this issue by updating the checklist above and giving feedback in this issue. The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. If possible create issues (and cross-reference) in the submission's repository to avoid too specific discussions in this review thread.

If you have any questions or concerns please let me know.

kdorheim commented 4 years ago

@sebkopf, isoreader is well written, well documented, and fulfills an important need in supporting reproducible open-source science. As indicated by the manuscripts that have cited this package it has already been useful to the community. The paper clearly describes the need for this package and is well written for a general audience (as a non-specialist I really appreciated that), however, it seems like the authors could have referenced citations for some of the information in the summary (i.e. the comments about availability and variability of isotopes). This being said if these generally known facts in the community, then no citations are needed. I've opened some issues on the isoreader repo related to the JOSS guidelines, which I believe are relatively minor fixes.

mikldk commented 3 years ago

@whedon add @r-barnes as reviewer

whedon commented 3 years ago

OK, @r-barnes is now a reviewer

mikldk commented 3 years ago

@r-barnes: Thanks for agreeing to review. Please carry out your review in this issue by updating the checklist above and giving feedback in this issue. The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. If possible create issues (and cross-reference) in the submission's repository to avoid too specific discussions in this review thread.

If you have any questions or concerns please let me know.

mikldk commented 3 years ago

@kdorheim, @r-barnes, would you mind giving a brief status of your progress?

kdorheim commented 3 years ago

Hi @mikldk, I've gone through the checklist, opened issues when relevant/marked others as complete.

JOSS plain text license files isoverse/isoreader#141 Open JOSS Contribution and authorship isoverse/isoreader#142 Open JOSS Installation instructions isoverse/isoreader#143 Open JOSS Automated tests isoverse/isoreader#144 Open JOSS Example usage isoverse/isoreader#145

sebkopf commented 3 years ago

thank you @kdorheim! pull request for the changes to address the raised issues is here: https://github.com/isoverse/isoreader/pull/148

whedon commented 3 years ago

:wave: @kdorheim, please update us on how your review is going.

mikldk commented 3 years ago

@kdorheim, @r-barnes, would you mind giving a brief status of your progress?

kdorheim commented 3 years ago

@whedon I've been working with @sebkopf on resolving the only remaining open issue I opened as part of my review.

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
mikldk commented 3 years ago

@r-barnes, would you mind giving a brief status of your progress?

mikldk commented 3 years ago

@r-barnes, would you mind giving a brief status of your progress?

r-barnes commented 3 years ago

Apologies; I'll put this review at the top of my todo list for tomorrow.

mikldk commented 3 years ago

Apologies; I'll put this review at the top of my todo list for tomorrow.

Thanks, @r-barnes . Can you give any indication of the expected time frame for your review?

kdorheim commented 3 years ago

@mikldk, @sebkopf has done an excellent job addressing all of the issues I opened related to this package, I believe that my review is complete 👍

mikldk commented 3 years ago

Thanks, @kdorheim and @sebkopf .

@r-barnes, would you mind giving a brief status of your progress?

r-barnes commented 3 years ago

For whatever reason, I'm unable to see the LICENSE file. However, CRAN can be a bear to work with and I see the license info in the DESCRIPTION file that CRAN uses, so I feel myself satisfied.

r-barnes commented 3 years ago

Sorry I'm so behind on this!

I've just finished looking through the repo. In general, things look pretty good. I've opened a few issues, as above. I reiterate the main points below:

  1. Installation did not complete for me. It looks like some dependencies are not on CRAN and I didn't immediately see instructions for overcoming this.
  2. This isn't my field, so I don't have appropriate input data available, but it looks highly plausible that the software does what is claimed. In particular, the impressively thorough documentation and the presence of unit tests are good evidence. However, the authors do claim the software will read a number of file types, but only have tests for a subset of these. From a testing perspective alone, it would be good to fill this gap. In this case, it's also important for evaluating functionality (at least for me).
  3. Example usage. If I click around I can find a lot of complex examples. I think the README itself would be facilitated by having an extremely simple example to get the reader started (similar to what I do here, perhaps). I feel like a simple example helps sell viewers on the idea of looking in depth.

I think once these are taken care of, this should be good to go.

mikldk commented 3 years ago

Thanks for the progress, @r-barnes . @sebkopf , please update us here when you have addressed the comments/issues put forward by @r-barnes .

sebkopf commented 3 years ago

Thank you @r-barnes for going through this. A few quick questions for clarification

For whatever reason, I'm unable to see the LICENSE file. However, CRAN can be a bear to work with and I see the license info in the DESCRIPTION file that CRAN uses, so I feel myself satisfied.

Thank you for checking. The LICENSE is in the Joss branch at https://github.com/isoverse/isoreader/tree/joss, waiting for all edits from the review process to merge.

  1. Installation did not complete for me. It looks like some dependencies are not on CRAN and I didn't immediately see instructions for overcoming this.

Could you copy which dependencies it is having trouble with? I'm testing on Windows, Linux and Mac OS with GitHub actions but could not find issues with dependencies except for the unreleased R development version (4.1) where devtools is not yet available. I suspect it might be digest which has in the past sometimes been a problem on windows (instructions for reinstall in the readme) or the bioconductor dependency rhdf5 if BiocManager is not pulled correctly from CRAN. We can definitely add some troubleshooting guidelines for these if they are what is causing the problem.

  1. This isn't my field, so I don't have appropriate input data available, but it looks highly plausible that the software does what is claimed. In particular, the impressively thorough documentation and the presence of unit tests are good evidence. However, the authors do claim the software will read a number of file types, but only have tests for a subset of these. From a testing perspective alone, it would be good to fill this gap. In this case, it's also important for evaluating functionality (at least for me).

The unit tests for the different file types (continuous flow, dual inlet and scan data) are in different test files to keep them easier to maintain with several example files included for all supported data types (for exactly the reason you mentioned, important to ensure functionality across the board):

Note that not all of the example files are included on CRAN because of the limits on package file size but are always tested as part of the GitHub continuous integration. Also, some file extensions have more test files than others because we found more variability in the format (despite identical extension). Would you recommend additional tests or are these sufficient?

  1. Example usage. If I click around I can find a lot of complex examples. I think the README itself would be facilitated by having an extremely simple example to get the reader started (similar to what I do here, perhaps). I feel like a simple example helps sell viewers on the idea of looking in depth.

We originally had this in the README but it got too cluttered so decided to put it into a Quick Start vignette, which is at https://isoreader.isoverse.org/articles/quick_start.html and covers some very basic code usage for the package. Would you suggest including this in the README as well or would it suffice to make a clear How to get started section in the readme that links to this vignette? (I realize we completely forgot to link this in the readme!)

Thank you very much for your time!

sebkopf commented 3 years ago

Hi @r-barnes, just a quick note that the README updates (and all other JOSS review related updates) are now merged into master and reflected at https://isoreader.isoverse.org/ (including clear links to the vignettes and a snippet of example code). Let me know if you have any other questions regarding the testing details mentioned above or if this all sounds good for moving towards publication.

Thank you very much.

sebkopf commented 3 years ago

@mikldk and @r-barnes : I think all issues raised by @r-barnes have now been addressed. Please let us know if you still see anything missing. Thank you very much for all your work on reviewing this. Best regards.

mikldk commented 3 years ago

@r-barnes still needs to check a few boxes. Can you give a status update?

mikldk commented 3 years ago

@r-barnes still needs to check a few boxes. Can you give a status update?

mikldk commented 3 years ago

@r-barnes still needs to check a few boxes. Can you give a status update?

sebkopf commented 3 years ago

Please let me know if there's anything I can do to help to get this wrapped up @mikldk @r-barnes . Much appreciate everyone's time working on this.

r-barnes commented 3 years ago

Sorry for going MIA. I've noticed github's notification system has improved, so hopefully I can make better use of that in the future. I'll take another look and see if we can get this wrapped up.

mikldk commented 3 years ago

@r-barnes, can you please give a brief status of your review? This is not to rush you, merely to give me an impression of the progress and time-frame.

mikldk commented 3 years ago

@r-barnes, can you please give a brief status of your review? This is not to rush you, merely to give me an impression of the progress and time-frame.

sebkopf commented 3 years ago

Hi @mikldk and @r-barnes - could you give me an update when you get the chance? It has been ~2 months since we addressed the last concerns and I was wondering if there is anything more that we need to do to get this published. I appreciate your time and effort on this publication very much. Best regards

mikldk commented 3 years ago

@sebkopf I understand your request for an update - this slipped through my filter, sorry.

@r-barnes Please finalise your review as soon as possible.

r-barnes commented 3 years ago

I hope to have time this weekend or late Friday night :-(

r-barnes commented 3 years ago

After working through the software again and reviewing responses to prior comments, I think this is good to go and put my Reviewer's Mark of Approval™ on it. My apologies again for being so late on this.

mikldk commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

mikldk commented 3 years ago

@sebkopf:

mikldk commented 3 years ago

@sebkopf, can you please give a brief status of the bullets above?

sebkopf commented 3 years ago

Hi @mikldk . Got zenodo finally figured out:

The release version is 1.3.0 and the archive DOI is 10.5281/zenodo.4724083:

@whedon generate pdf

sebkopf commented 3 years ago

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@mikldk This all looks great, happy to approve this proof. Is there anything else you need me to do? As mentioned above, zenodo DOI is 10.5281/zenodo.4724083

mikldk commented 3 years ago

@whedon set v1.3.0 as version

whedon commented 3 years ago

OK. v1.3.0 is the version.

mikldk commented 3 years ago

@whedon set 10.5281/zenodo.4724083 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4724083 is the archive.

mikldk commented 3 years ago

@whedon accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...