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]: py_SBeLT: A Python software package for stochastic sediment transport under rarefied conditions #4282

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@szwiep<!--end-author-handle-- (Sarah Zwiep) Repository: https://github.com/szwiep/py_SBeLT Branch with paper.md (empty if default branch): Version: v1.0.2 Editor: !--editor-->@kbarnhart<!--end-editor-- Reviewers: @pfeiffea, @tdoane Archive: 10.6084/m9.figshare.19967552.v3

Status

status

Status badge code:

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

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

@pfeiffea & @tdoane, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @kbarnhart 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 ✨

Checklists

📝 Checklist for @tdoane

📝 Checklist for @pfeiffea

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.15 s (177.5 files/s, 92229.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                             5            989              0           7950
Python                           7            491            913           1348
Markdown                         9            167              0            468
TeX                              1             15              0            180
Jupyter Notebook                 2              0            839             80
YAML                             2              5             30             38
-------------------------------------------------------------------------------
SUM:                            26           1667           1782          10064
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 1219

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

OK DOIs

- 10.1103/PhysRevE.74.011302 is OK
- 10.1017/S0022112007008774 is OK
- 10.1029/2009JF001260 is OK
- 10.1080/00221686.2019.1702594 is OK
- 10.14288/1.0349138 is OK
- 10.1002/2015JF003552 is OK
- 10.1029/2012JF002352 is OK
- 10.1002/2016JF003833 is OK
- 10.5194/esurf-9-629-2021 is OK
- 10.5194/esurf-6-1089-2018 is OK
- 10.1061/9780784408148.ch03 is OK
- 10.1029/2012JF002353 is OK
- 10.1002/2014RG000474 is OK
- 10.1029/2019WR025116 is OK

MISSING DOIs

- None

INVALID DOIs

- None
kbarnhart commented 2 years ago

@editorialbot set v1.0.1 as version

editorialbot commented 2 years ago

Done! version is now v1.0.1

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:

kbarnhart commented 2 years ago

👋🏼 @pfeiffea @tdoane this is the review thread for the paper. All of our communications about this review will happen here from now on.

At the top of the thread is a comment with key information about the review process. Please read it carefully. If you have issues as you go about your review, let me know, and I'll provide guidance and assistance.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews/issues/4282 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

I'll set an automatic reminder for four weeks from now.

Please feel free to ping me (@krbarnhart or krbarnhart@usgs.gov) if you have any questions/concerns.

Thank you for contributing a review to JOSS.

tdoane commented 2 years ago

Review checklist for @tdoane

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

szwiep commented 2 years ago

Hi all, I forgot to include the updated paper.md in the pre-review revisions. I realized my mistake and just pushed it to the main branch now. Hopefully this doesn't cause any difficulties. @kbarnhart should I re-generate the pdf?

kbarnhart commented 2 years ago

@szwiep yes, at any time you can re-generate the PDF.

kbarnhart 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:

pfeiffea commented 2 years ago

@editorialbot generate my checklist

kbarnhart commented 2 years ago

@editorialbot remind @pfeiffea in two weeks

editorialbot commented 2 years ago

Reminder set for @pfeiffea in two weeks

kbarnhart commented 2 years ago

@editorialbot remind @tdoane in two weeks

editorialbot commented 2 years ago

Reminder set for @tdoane in two weeks

pfeiffea commented 2 years ago

@kbarnhart I'm having a clueless moment... any idea why the @ editorial bot didn't generate my checklist, above?

kbarnhart commented 2 years ago

@pfeiffea no clue why. I made one for you by hand.

@arfon - Flagging that editorialbot didn't make a checklist for @pfeiffea when we expected it would.

pfeiffea commented 2 years ago

@kbarnhart thanks for doing that-- and then I realized that I could have just copied @tdoane 's. In any case, I can't check boxes on yours, and when I tried again just now it worked. Shrug.

Review checklist for @pfeiffea

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

pfeiffea commented 2 years ago

First off, well done @szwiep (and @smchartrand). This is a very well organized, well documented bit of software. It is a simple and elegant little tool. My edits are relatively minor. I have a few lingering “to-dos” on my review checklist, which I’ll deal with in the coming days.

Basic Usage Notebook:

This is an excellent notebook. It is not verbose, but gives me a great sense for how to use and interpret results of pySBeLT. Minor suggestion: It would be great to add the default parameters to the “parameters and running” table.

Data Storage Notebook:

Accessing Final Metrics section you’re missing a “.hdf5” at the end of the file name in the 2nd and 3rd to last code blocks. Make sure to restart the kernel and clear all outputs (or run all) in your final submission. The version up on your repo currently shows that you’ve re-run cells several times without restarting.

Manuscript:

Ln 12: I’d suggest editing to “… a function of local water flow conditions and the grain size distribution of the bed material.” Ln 12-14: The sentence that starts “A predictive approach…” seems like your opportunity to succinctly state the gap pySBeELT is trying to fill. But, as written, it feels a bit muddled (“the approach introduces challenges”). Do you mean something like “This standard approach to modeling bedload transport obscures(? Neglects?) the mechanistic reality of sediment transport: the bed material flux is the integrated transport of individual stochastic particle motions.” Ln 17: Instead of “examine connections”, how about “reveal the relationship” or “illustrate the relationship” (the user will be the one examining, the tool will make the examination possible)? Ln 38: “motivated by a..” suggests that your work was motivated by some specific earlier published work. If so, cite. Or maybe “pySBeLT takes a birth-death….. approach to modeling bedload transport”. Ln 46: set_diam should have quotes around it, right? ‘set_diam’ Ln 51-54: I think these last two sentences should be combined or re-ordered. Figure 1. Add a flow direction arrow. Figure 2. The lower panel is too small to read. Add the units of particle flux to the y axis of the upper plot.

Fix citation issues: ? in Wainwright 2014 and Wu 2019 Some first names or initials included in the in-text citations.

THEORY.md

Entrainment section, second paragraph has some odd number typos interspersed in the text. Take a look. @kbarnhart should this section be a part of the paper? It seems to me that it describes both the rationale and function of the model, so maybe? Or is it better to keep separate?

smchartrand commented 2 years ago

Thanks @pfeiffea. These are all super helpful suggestions. We appreciate your time and energy reviewing our submission.

kbarnhart commented 2 years ago

@pfeiffea many thanks for your review! 🎉

Regarding your question:

@kbarnhart should this section be a part of the paper? It seems to me that it describes both the rationale and function of the model, so maybe? Or is it better to keep separate?

I tend to recommend that theory lives with the main documentation (here, the file theory.md) rather than in the paper because the theory is then more closely tied with what a typical user is looking at when they are learning to use the software. In contrast, the paper would contain the type of information that a potential user might use to determine if this software could solve their problem.

I agree with you that in this case, the theory.md file includes both rationale and function (e.g., discussion of results of model testing). A more traditional theory document would probably just have the rationale and mathematical description. A second document might discuss how the theory interacts with the implementation (e.g., the discussion of how discretization interacts with entrainment "This means particle entrainment is treated as independent events between the 'num_subregions' (see README.md and paper.md) and between each iteration.")

If either you (@pfeiffea) or @tdoane found that it was hard to "see" the theory as described by "theory.md", then it may be worth recommending to @szwiep and co-authors to revise. If not, I think that it is fine as it is.

editorialbot commented 2 years ago

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

editorialbot commented 2 years ago

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

kbarnhart commented 2 years ago

@pfeiffea - Feel free to ignore the automatic reminder as you've already provided your update. It occurs if any checkboxes are unchecked.

tdoane commented 2 years ago

@editorialbot @kbarnhart, I thought I had uploaded my comments on Saturday! will do that now!

editorialbot commented 2 years ago

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

@editorialbot commands

tdoane commented 2 years ago

Hi @szwiep, @smchartrand, and @kbarnhart,

Here's my relatively minor comments. I have not used GitHub much in a collaborative way, so please let me know if I should post these differently!

This is an impressive piece of work that will provide a useful tool for teaching and research related to stochastic sediment transport. We can so simply state that sediment transport is the result of distributions of particle travel distance and particle rest times. However, when it comes to modeling this behavior we quickly run in to many complications related to keeping track of particle positions, burial, and particle history. This is a very clean treatment of the problem and the authors have clearly thought deeply about it. I have relatively minor comments that mostly relate to the paper. I want to emphasize that I think this is an excellent tool, and in particular that there is potential to use this to effectively illustrate the value of probabilistic approaches and the cosnequences of different distributions.

Paper: Why pySBeLt? It is an acronym?

Statement of need, 1st paragraph: roughly twice the shear stress threshold for motion? I assume?

Statement of need, 5th paragraph: For the periodic boundary condition: If a particle passes the lower boundary, it is placed at the upstream boundary, but does not continue with the motion that passed it over the downstream boundary, right? So the travel distance that sends it over the boundary is not recorded except that it exceeds a certain distance?

Statement of need: What is the significance of the length of the flume to the distribution of particle travel distances. Given the quasi periodic boundary conditions, this might warrant a brief discussion.

Statment of need: The authors end with a statement about the utility in classrooms. I completely agree and wonder if it could/should be stated near the top.

Theory:

Theory: a normal distribution will have negative travel distances. are these allowed? or is the distribution forced so that particle travel distances are all down stream?

Theory: What about the possibility of adding more probability distributions? Insofar as this has potential to be an excellent teaching tool, do the authors think that there is value in providing more distributions? In particular, a Pareto distribution has a minimum value that may address the issue of isolated piles forming for small or zero mode and it has a heavy tail where the variance becomes undefined for certain shape parameters. This might be an opportunity to illustrate the consequences of heavy-tailed distributions and the influence on the time-series of sediment flux.

Theory, 4th paragraph: "posiitons"

Code: The only issue was the same one raised by @pfeiffea

smchartrand commented 2 years ago

Thanks very much @tdoane for your supportive and helpful suggestions. We would also like to thank you and @pfeiffea for completing your reviews on a timely manner and during a very busy time of the year. We cannot thank you both enough.

kbarnhart commented 2 years ago

@pfeiffea and @tdoane many thanks for constructive reviews!

@smchartrand and @szwiep - when you have finished addressing the comments, please ping me here and I'll review the changes. If you have questions for me or the reviewers, please ask as you go about making changes.

Once I've reviewed the revised submission, I'll ask that the two reviewers re-visit their reviews and attest that all changes have been made.

Thanks!

smchartrand commented 2 years ago

@kbarnhart many thanks for the instructions on how to proceed. We will address the comments and let you know when the revised repository is ready to go. Look to hear back from us sometime next week.

Thanks to all for the most constructive review process in my research career to date.

szwiep commented 2 years ago

Hi @kbarnhart, @smchartrand and I have completed the suggested changes. Shawn has also prepared a short response to comments document that can hopefully be some help during the re-review process!

kbarnhart 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.1103/PhysRevE.74.011302 is OK
- 10.1017/S0022112007008774 is OK
- 10.1029/2009JF001260 is OK
- 10.1080/00221686.2019.1702594 is OK
- 10.14288/1.0349138 is OK
- 10.1002/2015JF003552 is OK
- 10.1029/2012JF002352 is OK
- 10.1002/2016JF003833 is OK
- 10.5194/esurf-9-629-2021 is OK
- 10.5194/esurf-6-1089-2018 is OK
- 10.1061/9780784408148.ch03 is OK
- 10.1029/2012JF002353 is OK
- 10.1002/2014RG000474 is OK
- 10.1029/2019WR025116 is OK

MISSING DOIs

- None

INVALID DOIs

- None
kbarnhart 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:

smchartrand commented 2 years ago

@kbarnhart - I have addressed your comments. Apologies for the lingering issues.

kbarnhart 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:

kbarnhart commented 2 years ago

@pfeiffea and @tdoane - I think that @szwiep and @smchartrand have addressed your concerns through revision.

@tdoane - please indicate whether you think the authors work is sufficient and ready to be published.

@pfeiffea - please do the same, but also evaluate whether any remaining checkboxes can be checked.

If possible, try to complete this stage of review by the end of this week (May 20th).

Let me know with any questions.

kbarnhart commented 2 years ago

:wave: @pfeiffea and @tdoane a friendly reminder to revisit your review of py_SBELT.

tdoane commented 2 years ago

Hi @kbarnhart @szwiep and @smchartrand ,

I've looked over the changes and I believe that the authors have addressed most of my comments. I have one remaining comment:

With regard to the discussion of heavy tails for log-normal and Pareto distributions in THEORY.md. The way it is phrased, it reads like the log-normal can also have undefined variance for certain parameters. I'm not quite sure that this is correct - though I may be wrong. I just want to bring it to the authors attention to double check. But otherwise, they have addressed all of my comments! Congratulations on a great piece of work!

smchartrand commented 2 years ago

Thanks @tdoane. Very good point, and not the intention of the edit. I will separate that sentence into two statements so there is no uncertainty around what we mean. I will make that edit today.

Thanks for your time and constructive feedback. It is much appreciated.

smchartrand commented 2 years ago

@kbarnhart @tdoane I have edited the sentence in the Theory.md which @tdoane discusses in the post today. I simply removed the clause pertaining to the lognormal distribution - the revised sentence reads: The Pareto distribution also has the advantage of being heavy-tailed where the variance becomes undefined for certain shape parameters.

thanks.

pfeiffea commented 2 years ago

Thanks for the prod @kbarnhart.

I'm happy with the changes the authors have made addressing my comments.

The one final checkbox on my review list was testing. I think this was my programming naivete, but simply following the Testing your installation code didn't work for me: python -m unittest src/tests/test_* gave an import error. Should that wildcard '*' work as written? I'm running it on Windows 10 in anaconda powershell prompt, in case that matters. Running python -m unittest src/tests/test_utils.py and python -m unittest src/tests/test_logic.py ran for me, though.

The latter, however, gives a bunch of errors: ERROR:root:No left supporting particle at 0.75for particle 0.0, all for particle 0.0.

@smchartrand and @szwiep I suspect there's something special with particle 0.0..? Perhaps you just need an exception for that error?

szwiep commented 2 years ago

Hi @pfeiffea! I'll look into the issue with the python -m unittest src/tests/test_* call tonight. I'm hoping it's just an incompatibility with Windows. If you have a chance, could you post the import errors you're getting?

As for the particle 0.0 business, those are forced errors in the TestFindSupports.test_no_supports_available_returns_value_error unit test. It tests that an error is thrown if a model particle doesn't have 2 supporting particles. So in this case, the errors being thrown are a good sign. I could play around with the output configuration to disable errors messages during unit testing, if you think that could help with clarity? Either way I'll update the testing section to more clearly describe the output one should expect. Thank-you for the input!

szwiep commented 2 years ago

Hi again @pfeiffea, I believe I've addressed your comments in pull requests 23 and 24 in the project repository. The new unittest commands appear to be working on both Linux and Windows systems and the stderr from passing tests have been suppressed to help make the results more clear to users. Let me know if these changes are appropriate. Thanks again!

pfeiffea commented 2 years ago

@szwiep Those changes did the trick!

I checked the "tests" box on the review checklist. @kbarnhart from my perspective, @szwiep and @smchartrand are good to go 💯