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]: HyRiver: Hydroclimate Data Retriever #3175

Closed
40 tasks done
whedon opened this issue Apr 15, 2021 · 71 comments
Closed
40 tasks done

[REVIEW]: HyRiver: Hydroclimate Data Retriever #3175

whedon opened this issue Apr 15, 2021 · 71 comments
Assignees

Comments

@whedon
Copy link

whedon commented Apr 15, 2021

Submitting author: @cheginit (Taher Chegini)
Repository: https://github.com/cheginit/HyRiver
Version: v0.11
Editor: @kthyng
Reviewers: @raoulcollenteur, @arbennett
Archive: 10.5281/zenodo.5602114

⚠️ JOSS reduced service mode ⚠️

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

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

@arbennett & @raoulcollenteur, 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 @kthyng 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 @arbennett

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

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?
  • Contribution and authorship: Has the submitting author (@cheginit) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

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 functionality 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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @raoulcollenteur

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

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?
  • Contribution and authorship: Has the submitting author (@cheginit) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

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 functionality 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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
@whedon
Copy link
Author

whedon commented Apr 15, 2021

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

⚠️ JOSS reduced service mode ⚠️

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.

⭐ Important ⭐

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
Copy link
Author

whedon commented Apr 15, 2021

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3334/ORNLDAAC/1840 is OK
- 10.3133/fs20193051 is OK
- 10.3133/fs20203033 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@whedon
Copy link
Author

whedon commented Apr 15, 2021

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.08 s (307.3 files/s, 22541.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
reStructuredText                12            232            212            564
YAML                             6             21             16            199
SVG                              1              0              0            162
Python                           2             35             55            122
Markdown                         1             10              0            113
make                             2             18              9             76
DOS Batch                        1              8              1             26
TeX                              1              2              0             26
-------------------------------------------------------------------------------
SUM:                            26            326            293           1288
-------------------------------------------------------------------------------


Statistical information for the repository 'f3d8ba1c0ba6cb9fae2ee0b7' was
gathered on 2021/04/15.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
cheginit                         8         18139          17927          100.00

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
cheginit                    212            1.2          0.0               22.64

@whedon
Copy link
Author

whedon commented Apr 15, 2021

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

@whedon
Copy link
Author

whedon commented Apr 29, 2021

👋 @evanleeturner, please update us on how your review is going (this is an automated reminder).

@whedon
Copy link
Author

whedon commented Apr 29, 2021

👋 @raoulcollenteur, please update us on how your review is going (this is an automated reminder).

@kthyng
Copy link

kthyng commented May 13, 2021

just a friendly review reminder here, @evanleeturner and @raoulcollenteur

@raoulcollenteur
Copy link

raoulcollenteur commented May 17, 2021

Started on the Review today, interesting work! I will edit this comment over the next couple of days.

Summary
HyRiver is a collection (software stack) of six python packages that aim to improve the workflow of data collection and processing for hydrological and climatological data from public web services. This is an important and often time-consuming step in hydrological studies, and I congratulate the Authors on their work. Although several alternatives are available (also listed in the paper), this stack seems to provide a unified access to many data sources in the US. The stack is subdivided over several (8?) GitHub repositories, one of which combines all the packages (https://github.com/cheginit/HyRiver). The package is currently actively maintained and looking at some of the user statistics there is a clear demand for this package. The software stack is a significant scholarly contribution in my opinion and suitable for publication in JOSS.

Package Organization
My only real concern is the dispersion of the information and code. The packages, information, and support structures are subdivided over several GitHub repositories. I think it would be good to (at least) put all of these under a single GitHub organization and try to reduce the dispersion of information a bit. Documentation is already provided from a single ReadTheDocs website, which is a nice entry point for new users. It could help to do the same for the user support structures (e.g., Code Issues, User questions), which now seem spread over all repositories. Maybe the Authors prefer it the way it is, but I can imagine it is more work to maintain and keep track of user discussions.

Would it not be a better idea to combine all the packages into a single Python packages and have the current packages as sub-packages of hyriver? Given the limited amount of code contained in the individual packages I think this would not be a bad option and it would probably reduce the maintenance cost in the long term. Is there a specific reason different packages were created instead of a single one? A related question I have is if it is possible to use the packages independently. If so, this would be worth mentioning in the docs installation guide. If any case, it could be considered to put all packages into a single python package and use sub-packages to structure.

Here's a list of minor suggestions to improve the package, JOSS paper and documentation (may I add more later):

  • I think most of the packages are only applicable in certain geographic areas (US?), this should be made explicit in the Docs and JOSS paper.
  • Consider adding a page with the features of each package to the documentation website (hyriver.readthedocs.io)
  • The Contributing page seems outdates and can be updated: https://hyriver.readthedocs.io/en/latest/contributing.html
  • I could not easily find a location where user can "seek support", maybe using a central GitHub Discussion or Slack is an idea?

I am still trying to run the examples from https://github.com/cheginit/HyRiver-examples but will need some more time for that. I get lots of errors when trying to import packages, will post some here later.

  • import pydaymet: AttributeError: 'LGEOS360' object has no attribute 'GEOSBufferWithParams'
  • import pygeohydro: AttributeError: module 'pyproj' has no attribute 'crs'
  • import py3dep: AttributeError: module 'pyproj' has no attribute 'crs'

Great work!

Cheers,
Raoul

@cheginit
Copy link

@raoulcollenteur Thanks for accepting to review this submission and your comments.

My only real concern is the dispersion of the information and code. The packages, information, and support structures are subdivided over several GitHub repositories. I think it would be good to (at least) put all of these under a single GitHub organization and try to reduce the dispersion of information a bit. Documentation is already provided from a single ReadTheDocs website, which is a nice entry point for new users. It could help to do the same for the user support structures (e.g., Code Issues, User questions), which now seem spread over all repositories. Maybe the Authors prefer it the way it is, but I can imagine it is more work to maintain and keep track of user discussions.

Would it not be a better idea to combine all the packages into a single Python packages and have the current packages as sub-packages of hyriver? Given the limited amount of code contained in the individual packages I think this would not be a bad option and it would probably reduce the maintenance cost in the long term. Is there a specific reason different packages were created instead of a single one?

The software stack was initially a single package called hydrodata. As the package started to grow and I added support for more service and new functionalities, the maintenance became more difficult. So I decided to break it down into six self-contained packages, each of which has a limited scope and serves different target users. For example, some users might only want to have access to river network data, so instead of installing a hefty package with many dependencies and irrelevant functions, they can just install PyNHD that offers what they need. Also, from development point of view (for the core developers as well as contributors), working on new functionalities and tracking down issues become easier as the code base is smaller for each package and better organized. The only downside is more work for releasing six packages instead of one which is not big of an issue overall.

Moreover, the packages are divided into two categories: Low- and high-level APIs. PyGeoOGC and PyGeoUtils (and the recently added package called async_retriever) are the engines, if you will, that the other packages use to connect to specific web services. The other packages are specific to the public web services that are provided by US agencies.

A related question I have is if it is possible to use the packages independently. If so, this would be worth mentioning in the docs installation guide. If any case, it could be considered to put all packages into a single python package and use sub-packages to structure.

Yes, the packages are standalone. The README of each one of the repositories contains descriptions, instructions, and examples that are specific to that package. I will modify the paper as well as repositories' README to reflect this explicitly.

I think most of the packages are only applicable in certain geographic areas (US?), this should be made explicit in the Docs and JOSS paper.

Yes, currently only US is supported but contributions are welcome as the low-level packages are generic and can be used to access any of the supported web service protocols. I will make this more explicit in the paper and repositories.

Consider adding a page with the features of each package to the documentation website (hyriver.readthedocs.io)

I thought about this when I was designing the website. Since each package has a repository with a README that has all the relevant information, instead of repeating the same thing in the website, I decided to include a one-line description of each package and provide the link to each repository. Additionally, the example gallery includes most of the things that are in READMEs and even more, so in my opinion including them again in separate pages will be repetitive.

The Contributing page seems outdates and can be updated: hyriver.readthedocs.io/en/latest/contributing.html

I will take care of it.

I could not easily find a location where user can "seek support", maybe using a central GitHub Discussion or Slack is an idea?

The idea is that users can open issues for packages in their corresponding repositories or use the main repository (HyRiver). Users have been using both. I just added a discussions tab to the HyRiver's repository.

I am still trying to run the examples from cheginit/HyRiver-examples but will need some more time for that. I get lots of errors when trying to import packages, will post some here later.

* [ ]  import pydaymet: AttributeError: 'LGEOS360' object has no attribute 'GEOSBufferWithParams'

* [ ]  import pygeohydro: AttributeError: module 'pyproj' has no attribute 'crs'

* [ ]  import py3dep: AttributeError: module 'pyproj' has no attribute 'crs'

It would be great and appreciated if you can provide more details and, perhaps, open an issue so I can reproduce and fix them.

By the way, I am working on a new release as users reported some issues after xarray released a new version recently that included some breaking changes. I have already took care of the issues in the main branches of the affected repositories and will release a new version soon.

cheginit pushed a commit to hyriver/hyriver.github.io that referenced this issue May 27, 2021
cheginit pushed a commit to hyriver/hyriver.github.io that referenced this issue May 27, 2021
cheginit pushed a commit to hyriver/hyriver.github.io that referenced this issue May 27, 2021
@kthyng
Copy link

kthyng commented Jun 1, 2021

Hi @evanleeturner and @raoulcollenteur! Will you be able to continue your reviews soon?

@cheginit
Copy link

@kthyng Over the last two months (since the submission date), I have added enough new features and bug fixes to these libraries that prompt new releases. Would it be alright if I change the version from 0.10 to 0.11 in the first comment?

@kthyng
Copy link

kthyng commented Jun 16, 2021

@whedon set v0.11 as version

@whedon
Copy link
Author

whedon commented Jun 16, 2021

OK. v0.11 is the version.

@kthyng
Copy link

kthyng commented Jun 16, 2021

@cheginit I have updated your version, and we'll also update it at the end of the review process.

@kthyng
Copy link

kthyng commented Jun 16, 2021

@raoulcollenteur will you be able to work on your review soon? Just pinged @evanleeturner by email too.

@cheginit
Copy link

@kthyng Thanks, appreciate it.

@cheginit
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Jun 24, 2021

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

@kthyng
Copy link

kthyng commented Jun 25, 2021

@raoulcollenteur will you be able to work on your review soon? @evanleeturner said by email can work on it soon.

@kthyng
Copy link

kthyng commented Jul 12, 2021

@raoulcollenteur and @evanleeturner — just a friendly ping here about your reviews.

@cheginit
Copy link

@kthyng @raoulcollenteur @evanleeturner Please let me know if there are any issues that I need to address for this submission. Also, since April I made one minor release (0.11) that includes breaking changes and a new patch release (0.11.x) is around the corner. So please make sure to check out the latest version.

@kthyng
Copy link

kthyng commented Oct 26, 2021

@cheginit Can you verify what version of the software you want associated with the JOSS review? Currently it is on a minor revision number.

Also can you archive your software at a place like Zenodo and then report here the DOI? Please make sure to modify the metadata in the Zenodo (or other) archive so that it exactly matches your JOSS title and author list.

@kthyng
Copy link

kthyng commented Oct 26, 2021

@whedon generat pdf

@whedon
Copy link
Author

whedon commented Oct 26, 2021

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

@whedon commands

@kthyng
Copy link

kthyng commented Oct 26, 2021

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Oct 26, 2021

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

@kthyng
Copy link

kthyng commented Oct 26, 2021

@cheginit Your paper looks good! Just one issue: please fix up the capitalization in the references. You can preserve capitalization in your .bib file with {} around strings.

@cheginit
Copy link

@kthyng Great, thank you!

Can you verify what version of the software you want associated with the JOSS review? Currently it is on a minor revision number.

Yes, it only has major and minor versions. Since this is a software stack, the patch version is different for each package but minor versions are the same for all.

Also can you archive your software at a place like Zenodo and then report here the DOI? Please make sure to modify the metadata in the Zenodo (or other) archive so that it exactly matches your JOSS title and author list.

I just created a DOI on Zenodo: 10.5281/zenodo.5602113.

Your paper looks good! Just one issue: please fix up the capitalization in the references. You can preserve capitalization in your .bib file with {} around strings.

Can you please let me know which reference are you referring to that has capitalization issue?

@kthyng
Copy link

kthyng commented Oct 27, 2021

@cheginit So would you like to stay at v0.11?

In the final reference "North America" isn't capitalized. Maybe something in Thatcher et al should be capitalized, not sure about that one.

@kthyng
Copy link

kthyng commented Oct 27, 2021

@whedon set 10.5281/zenodo.5602114 as archive

@whedon
Copy link
Author

whedon commented Oct 27, 2021

OK. 10.5281/zenodo.5602114 is the archive.

@cheginit
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Oct 27, 2021

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

@cheginit
Copy link

@kthyng I just fixed the capitalization issue. Regarding the version, yes, 0.11 is still the latest version.

@kthyng
Copy link

kthyng commented Oct 27, 2021

@cheginit Ok everything looks good! We can wrap it up now!!

@kthyng
Copy link

kthyng commented Oct 27, 2021

@whedon accept deposit=true

@whedon whedon added accepted published Papers published in JOSS labels Oct 27, 2021
@whedon
Copy link
Author

whedon commented Oct 27, 2021

Doing it live! Attempting automated processing of paper acceptance...

@whedon
Copy link
Author

whedon commented Oct 27, 2021

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@whedon
Copy link
Author

whedon commented Oct 27, 2021

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.03175 joss-papers#2716
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.03175
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? Notify your editorial technical team...

@kthyng
Copy link

kthyng commented Oct 27, 2021

Congrats on your new publication @cheginit!! Many thanks to reviewers @raoulcollenteur and @arbennett for your time, hard work, and expertise!

@kthyng kthyng closed this as completed Oct 27, 2021
@whedon
Copy link
Author

whedon commented Oct 27, 2021

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.03175/status.svg)](https://doi.org/10.21105/joss.03175)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.03175">
  <img src="https://joss.theoj.org/papers/10.21105/joss.03175/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.03175/status.svg
   :target: https://doi.org/10.21105/joss.03175

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@cheginit
Copy link

@kthyng That's great news! I appreciate your help during the process.

@raoulcollenteur and @arbennett I would like to thank you for taking the time to reviewing my submission and providing constructive and encouraging comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants