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

JOSS Review #6

Closed
17 tasks done
jlarsen-usgs opened this issue Jan 23, 2023 · 5 comments
Closed
17 tasks done

JOSS Review #6

jlarsen-usgs opened this issue Jan 23, 2023 · 5 comments

Comments

@jlarsen-usgs
Copy link

jlarsen-usgs commented Jan 23, 2023

@barneydobson

I am beginning my review on WSIMOD for your JOSS publication and I will be tracking my review comments here and editing this issue as they come up, I should be finished doing my review by the end of this week. Feel free to address them all within this issue, or convert each of the comments to their own issues.

  • General reviews

    • I see that you have some tests set up for WSIMOD. It would be great to see these tests incorporated into a continuous integration system, such as github actions.
    • In your WSI docs you have a detailed summary and statement of need. Can you adapt a concise version of this text into your Readme.md file. The Welcome WSIMOD section would be a good place to add this information.
    • Add a requirements section in your Readme.md. What versions of python does your code support? What python packages are required or optionally required to use WSIMOD.
    • Community guidelines: A code of conduct, contributing guidelines, and either an issue template or a short statement about where to seek support for the software should be added to the reposity.
    • Add installation instructions to readme.md.
    • Update installation instructions in your api documentation (and in readme.md) to show installation with pip (pip install .). It'd be nice to add instructions on how to install directly from github for those users who aren't likely to dig into the source code (pip install https://github.com/barneydobson/wsi/archive/refs/heads/main.zip)
  • paper reviews:

    • The statement of need should expand on the gaps in scientific code/research this software is trying to fill and clarify what this software does and does not do.
      • How does this hydrologic simulator compare to other existing modeling software? What gaps does it fill?
    • Lines 22 and 23 state that you simulate "all parts of the water cycle". This should be replaced with a more detailed description of the processes that you simulate.
    • Please expand on the capabilities and limitations of your hydrologic simulator. I think it'd add a lot to the paper if you explained your approach of Arcs and Nodes to simulate fluxes and sinks/storage.
    • Optional: A short example application could really help illustrate the functionality of WSIMOD. I'd really consider consisely presenting one of your example problems (1-2 paragraphs and a figure) to illustrate the utility of the WSIMOD.
  • code reviews:

    • the demo problems for your documentation are currently missing data and not reproducible. All of the example problems are missing "timeseries_data.csv". Until this is fixed I can't really finish my review.
    • the demo problems are currently set up to generate your documentation tutorials. These could be adapted into either example scripts or interactive jupyter notebooks (in addition to keeping the current docs workflow) where the user does not have to reset the data_folder path variable to access the correct path to your data.
    • the bulk density of soil in your GrowingSurface module seems low to me (1300 kg/m3), The average bulk denity of soil is around 2650 kg/m3.

openjournals/joss-reviews#4996

@barneydobson
Copy link
Collaborator

barneydobson commented Jan 26, 2023

Thanks @jlarsen-usgs these are incredibly helpful comments - I'm working on all of these but to just reply about the timeseries_data.csv issue, it seems to me to be on the repo in this folder (https://github.com/barneydobson/wsi/tree/main/docs/demo/data/processed).

I should have made it more clear, but there is a comment that says when inputting the data_folder, that a different address should be used when running in Python on your machine (e.g., line 51 in https://github.com/barneydobson/wsi/blob/main/docs/demo/scripts/quickstart_demo.py)

As part of my revisions, I will make this the default data_folder address since I only need to use the address it is currently set as to create the documentation (you can switch over to the https://github.com/barneydobson/wsi/tree/joss_revisions branch where I have made the change) - but I am letting you know now so that you can continue looking at them on the main branch if you like.

@jlarsen-usgs
Copy link
Author

Thanks @barneydobson, I see that the file is in the WSIMOD repository. I'll update my copy of the repo to make sure I have the most recent version and retry later today.

@barneydobson
Copy link
Collaborator

barneydobson commented Jan 27, 2023

Hi @jlarsen-usgs

Just providing a quick update on these tasks, note that I have not yet been able to meet with co-authors so I haven't focussed on paper edits. Changes are currently on the https://github.com/barneydobson/wsi/tree/joss_revisions branch and summarised below.

  • Continuous integration included in a GitHub action.
  • Concise summary added to Readme and will be added to Welcome page of documentation.
  • Code of conduct added. Still figuring out the best approach to contributing guidelines/issue template.
  • Requirements (these are only needed to run the demos) have now been added to the readme.
  • Added install instructions to readme and edited according to your suggestion (but I might still iterate these to accommodate any changed requirements).
  • Default data_folder updated RE previous conversation.
  • I wanted to check the soil density with the person who's code the GrowingSurface module is based on.

@cheginit cheginit mentioned this issue Jan 27, 2023
11 tasks
@barneydobson
Copy link
Collaborator

Hi @jlarsen-usgs, thank you so much for your review, it has been incredibly helpful for us! Below I go through the open tasks that you have raised and provide links to locations where the suggestions have been implemented, and a bit of text in response if necessary.

  • In your WSI docs you have a detailed summary and statement of need. Can you adapt a concise version of this text into your Readme.md file. The Welcome WSIMOD section would be a good place to add this information.
    Addressed at: https://github.com/barneydobson/wsi/blob/main/README.md

  • Install/requirements

    • Add a requirements section in your Readme.md. What versions of python does your code support? What python packages are required or optionally required to use WSIMOD.
    • Add installation instructions to readme.md.
    • Update installation instructions in your api documentation (and in readme.md) to show installation with pip (pip install .). It'd be nice to add instructions on how to install directly from github for those users who aren't likely to dig into the source code (pip install https://github.com/barneydobson/wsi/archive/refs/heads/main.zip)

Addressed in:
https://github.com/barneydobson/wsi/blob/main/README.md
https://github.com/barneydobson/wsi/blob/main/requirements.txt
https://github.com/barneydobson/wsi/blob/main/requirements_demos.txt
https://github.com/barneydobson/wsi/blob/main/requirements_documentation.txt
https://github.com/barneydobson/wsi/blob/main/setup.py
https://barneydobson.github.io/wsi/installation/

Addressed in:
https://github.com/barneydobson/wsi/blob/main/docs/paper/paper.md
https://barneydobson.github.io/wsi/paper/paper/
Note that this is a reasonably small amount of text added, we are currently writing up a full research paper with a more complete explanation of theory.

  • Paper updates (capabilities)
    • Lines 22 and 23 state that you simulate "all parts of the water cycle". This should be replaced with a more detailed description of the processes that you simulate.
    • Please expand on the capabilities and limitations of your hydrologic simulator. I think it'd add a lot to the paper if you explained your approach of Arcs and Nodes to simulate fluxes and sinks/storage.
    • Optional: A short example application could really help illustrate the functionality of WSIMOD. I'd really consider consisely presenting one of your example problems (1-2 paragraphs and a figure) to illustrate the utility of the WSIMOD.

Capabilities/descriptions now added in:
https://barneydobson.github.io/wsi/component-library/
As previous, if you find it acceptable, a more detailed theoretical description will be addressed in a research paper

  • the demo problems are currently set up to generate your documentation tutorials. These could be adapted into either example scripts or interactive jupyter notebooks (in addition to keeping the current docs workflow) where the user does not have to reset the data_folder path variable to access the correct path to your data.
    Default folder now updated so that they are interactive

  • the bulk density of soil in your GrowingSurface module seems low to me (1300 kg/m3), The average bulk denity of soil is around 2650 kg/m3.
    Addressed in:
    https://github.com/barneydobson/wsi/blob/main/wsimod/nodes/land.py

@jlarsen-usgs
Copy link
Author

@barneydobson thank you for addressing my comments. I think this is ready to move ahead

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

No branches or pull requests

2 participants