search

openjournals / joss-reviews

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

[REVIEW]: nsink: An R package for flow path nitrogen removal estimation #4039

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@jhollist<!--end-author-handle-- (Jeffrey W Hollister) Repository: https://github.com/USEPA/nsink Branch with paper.md (empty if default branch): Version: v1.2.0 Editor: !--editor-->@crvernon<!--end-editor-- Reviewers: @jmp75, @ldecicco-USGS, @jmp75 Archive: 10.5281/zenodo.6341565

: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/3af12dd8d27ca1605958d4fd021deaa8"><img src="https://joss.theoj.org/papers/3af12dd8d27ca1605958d4fd021deaa8/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/3af12dd8d27ca1605958d4fd021deaa8/status.svg)](https://joss.theoj.org/papers/3af12dd8d27ca1605958d4fd021deaa8)

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

@jmp75 & @ldecicco-USGS, 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 @crvernon 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 @jmp75

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @ldecicco-USGS

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jmp75, @ldecicco-USGS 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 2 years ago

Wordcount for paper.md is 1017

whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.07 s (495.8 files/s, 65060.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               24            257            914           2179
Markdown                         5            101              0            349
TeX                              2             37              0            315
XML                              1              0              0            192
YAML                             3             25              2            121
Rmd                              2             70            239             54
-------------------------------------------------------------------------------
SUM:                            37            490           1155           3210
-------------------------------------------------------------------------------

Statistical information for the repository 'ad22c729492b33a55888e358' was
gathered on 2022/01/07.
No commited files with the specified extensions were found.
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.ecoleng.2010.02.006 is OK
- 10.32614/RJ-2018-009 is OK
- 10.18637/jss.v014.i06 is OK
- 10.1016/j.cageo.2004.03.012 is OK
- 10.32614/RJ-2016-014 is OK
- 10.1007/978-1-4614-7618-4 is OK
- 10.32614/RJ-2016-061 is OK
- 10.3133/ofr20191096 is OK
- 10.3390/rs11242971 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2 is INVALID
- 10.32614/RJ-2021-048 is INVALID
whedon commented 2 years ago

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

crvernon commented 2 years ago

:wave: @jhollist @jmp75 @ldecicco-USGS - the review takes place in this issue.

❗ Also, please don't forget to add a link to this review issue in any issues or pull requests you may generate in the https://github.com/USEPA/nsink repository. This will help everyone have a single point of reference.

crvernon commented 2 years ago

:wave: @jmp75 and @ldecicco-USGS have you found that you are able to modify the user checklist above? Let me know if you have any further questions about how to conduct your review, etc. Thanks!

ldecicco-USGS commented 2 years ago

@jhollist should we be installing via the USEPA repo, or your fork? (I don't know if it matters, but the readme says your fork) EDIT: I see the paper uses USEPA...

jhollist commented 2 years ago

@ldecicco-USGS sorry for the confusion, bit of a long story that I will spare you as to why I use the two...

It shouldn't matter. I have my local repo set to push to both locations so, in theory, they are identical. I use the EPA one in the paper and in DESCRIPTION so install from that. I do see now that I need to update my README installation instructions. Will do that shortly.

ldecicco-USGS commented 2 years ago

Say no more 😂

ldecicco-USGS commented 2 years ago

The package works as advertised, and the paper is clear to read. I don't see specific instructions on Community guidelines (I think this is generally a little different than the code of conduct that is included).

General thoughts, not required for approval, but might help some future developments: The tests look to me to be written to check generally that the data coming back from the functions are set up in the intended structure and without errors. Since the package is heavily based on the Kellogg paper, it would be neat to see the tests recreate and match the results from the paper.

crvernon commented 2 years ago

:mega: Mid-week rally!

👋 @jhollist - It looks like @ldecicco-USGS has requested a community guidelines section be added and has made several other helpful comments.

👋 @jmp75 - Were you able to initialize your review? Let me know if you have any questions.

Keep up the good work!

jmp75 commented 2 years ago

@crvernon I am dragging the chain a bit, apologies. I'll allocate solid time over the coming two days.

whedon commented 2 years ago

:wave: @ldecicco-USGS, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @jmp75, please update us on how your review is going (this is an automated reminder).

jhollist commented 2 years ago

@ldecicco-USGS and @crvernon thanks for the comments! There is a CONTRIBUTING.md but it is a little hidden inside of the .github folder. This is the defualt location for usethis::use_tidy_contributing which is what I used as an initial template for my contributing. This is one of the standard locations suggested by GitHub: https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors#adding-a-contributing-file. If JOSS has a different suggestion I am happy to move it.

Also, I will create an issue for a future enhancement that uses the Kellogg et al data for tests. This is a MOST excellent idea. Just will take some time to implement.

Thanks again!

ldecicco-USGS commented 2 years ago

Ack, sorry for not realizing that! Sure enough, it does make sense to put it in there.

jhollist commented 2 years ago

I will openly admit that I googled, "why is my contributing.md in the .github folder" prior to responding!

crvernon commented 2 years ago

@jmp75 - Please update me on the status of your review. Thanks!

crvernon commented 2 years ago

For record: I sent an email to @jmp75 today asking about the status of the review and the potential to conduct it.

jmp75 commented 2 years ago

@crvernon I have gone through the checklist and I do not have other suggestions than that already provided by Laura.

I could not edit the checklist though, the link provided to validate it (accept the invitation) says it does not exist or not anymore, or a message to that effect.

crvernon commented 2 years ago

@whedon add @jmp75 as reviewer

whedon commented 2 years ago

OK, @jmp75 is now a reviewer

crvernon commented 2 years ago

@jmp75 ok, you should have just re-received the invite and upon acceptance be able to check off your boxes in the above review thread.

crvernon commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

crvernon commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.ecoleng.2010.02.006 is OK
- 10.32614/RJ-2018-009 is OK
- 10.18637/jss.v014.i06 is OK
- 10.1016/j.cageo.2004.03.012 is OK
- 10.32614/RJ-2016-014 is OK
- 10.1007/978-1-4614-7618-4 is OK
- 10.32614/RJ-2021-048 is OK
- 10.32614/RJ-2016-061 is OK
- 10.3133/ofr20191096 is OK
- 10.3390/rs11242971 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1641/0006-35682002052[0129BSIPGO]2.0.CO2 URL is INVALID
crvernon commented 2 years ago

@jhollist Please see the following concerning the paper submission:

❗ Please remove your old license (https://github.com/USEPA/nsink/blob/main/LICENSE) as is not valid for JOSS submissions.

Additional:

Let me know once you fix the above and we will move along. Thanks!

jhollist commented 2 years ago

@crvernon Will get this finished up this week.

One quick question. The DOI for that reference is: 10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2 and it resolves correctly at https://doi.org/10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2. It appears that some of the special characters are getting lost. In the article proof, the square bracket are lost, but the URL resolves. As the DOI is correct in the .bib file, I am not sure what to do to get the JOSS DOI check to pass.

I will work on a Pkgdown site for it. It will have to be off my personal page though and not the USEPA repository as we do not have the ability to use github pages there.

crvernon commented 2 years ago

@jhollist I'll check on that DOI but if it's resolving we should be OK. Just let me know when you are ready for me to move forward. Thanks!

jhollist commented 2 years ago

@crvernon, first thanks for keeping this moving along.

I've made the edits, added mention of the CONTRIBUTING to the README, and have created a pkgdown site which is available via https://jhollist.github.io/nsink. I do not have admin rights on the https://github.com/usepa/nsink repository so haven't added this link to the About section. I can add a link to the README if you think that is good. FYI, I should have admin rights at some point in the next few months to that repo and can make changes then.

What are the next steps?

crvernon commented 2 years ago

@jhollist we are almost there! Next is just setting up the archive for your new release.

So here is what we have left to do:

I can then move forward with accepting the submission.

jhollist commented 2 years ago

@crvernon I have created a new release, uploaded that to Zenodo and have the DOI: 10.5281/zenodo.6341565

Anything else, just holler!

crvernon commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

crvernon commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.ecoleng.2010.02.006 is OK
- 10.32614/RJ-2018-009 is OK
- 10.18637/jss.v014.i06 is OK
- 10.1016/j.cageo.2004.03.012 is OK
- 10.32614/RJ-2016-014 is OK
- 10.1007/978-1-4614-7618-4 is OK
- 10.32614/RJ-2021-048 is OK
- 10.32614/RJ-2016-061 is OK
- 10.3133/ofr20191096 is OK
- 10.3390/rs11242971 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1641/0006-3568(2002)052[0129BSIPGO]2.0.CO2 URL is INVALID
crvernon commented 2 years ago

NOTE: the invalid DOI resolves correctly per the author's comment:

The DOI for that reference is: 10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2 and it resolves correctly at https://doi.org/10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2. It appears that some of the special characters are getting lost. In the article proof, the square bracket are lost, but the URL resolves. As the DOI is correct in the .bib file, I am not sure what to do to get the JOSS DOI check to pass.

crvernon commented 2 years ago

@editorialbot set 10.5281/zenodo.6341565 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.6341565

crvernon commented 2 years ago

@editorialbot set v1.2.0 as version

editorialbot commented 2 years ago

Done! version is now v1.2.0

crvernon commented 2 years ago

@jhollist - thanks for putting together a really nice software product! Thanks to @jmp75 and @ldecicco-USGS for a constructive and timely review!

I am recommending that your submission be accepted. An EIC will review this shortly and confirm final publication if all goes well.

crvernon commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.ecoleng.2010.02.006 is OK
- 10.32614/RJ-2018-009 is OK
- 10.18637/jss.v014.i06 is OK
- 10.1016/j.cageo.2004.03.012 is OK
- 10.32614/RJ-2016-014 is OK
- 10.1007/978-1-4614-7618-4 is OK
- 10.32614/RJ-2021-048 is OK
- 10.32614/RJ-2016-061 is OK
- 10.3133/ofr20191096 is OK
- 10.3390/rs11242971 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1641/0006-3568(2002)052[0129BSIPGO]2.0.CO2 URL is INVALID
crvernon commented 2 years ago

NOTE: the invalid DOI resolves correctly per the author's comment:

The DOI for that reference is: 10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2 and it resolves correctly at https://doi.org/10.1641/0006-3568(2002)052[0129:BSIPGO]2.0.CO;2. It appears that some of the special characters are getting lost. In the article proof, the square bracket are lost, but the URL resolves. As the DOI is correct in the .bib file, I am not sure what to do to get the JOSS DOI check to pass.

editorialbot commented 2 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/3036

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3036, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

jhollist commented 2 years ago

Thank you, @crvernon, @ldecicco-USGS, and @jmp75! JOSS is one of my favorite journals to submit to and review for!