~Thanks for the review, and Katy for posting here. I'm copying the review to this issue, so we can track and eventually respond to all the points on GitHub. I'll tag you both here to review our responses, once we have completed them.~

Review

from JOSS template

A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is? In the software paper but not in the documentation itself, that I could find. Thanks for checking on this. We have a high-level description of the software on the documentation home page and the the top of the repository readme, but these are not explicitly a "statement of need" --- Katy, do we need to add this to the documentation?

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified? I assume running through the ten-minute tutorial, etc. would count? In addition to the 10-minute tutorial, we have unit and integration testing for pyDeltaRCM, which you could run locally. We also run these tests in continuous integration, which you can see the results here.

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 I did not see this Thanks for checking on this. In the documentation, we include a Code of Conduct and a Contributing Guide, but the Contributing Guide was not very informative, so we have expanded the content (here in #221).

Specific Notes:

Installation:

I created a virtual environment in Anaconda, but there are some intermediate steps (I’m sure they are obvious to someone familiar with Python) to make the subsequent commands work. Thanks for pointing out these missing steps. Still, the installation here is tricky because every operating system has different commands, and users will have their own preferences for managing software installations. We've tried to cover some basics here like recommending Anaconda and development installations, but we feel that including complete instructions for installing a pure-Python package on various operating systems is beyond the scope for our documentation. Note though, that our CI testing installs and tests the software on three popular operating systems. We did add a line to the installation guide about activating the conda environment after creating it (in #218 here).
I tried the developer instructions but “git” is not recognized as a command for me and I have no idea how to do this step. Thanks for pointing out that we did not specify git as a dependency for developer installation. We think that installing and using git is outside the scope of Python package documentation, though. Still, we have changed the text here in #218 to specify the need to first install git and included a link to git installation instructions.
Following user instructions now. I had to figure out how to install pip, command to do so should be listed under install instructions Thank you for bringing this to our attention. We have included an instruction to install pip at this part of the documentation. (here in #218)

10-min tutorial

Instruction to first launch python is missing here --- I know this is something only a python novice would miss but would have helped me Sorry that this tripped you up. As an object-oriented package though, pyDeltaRCM works within a script or interactive Python session. We want to leave this up to the user to decide how they want to interact with the package, but have added a note (here in #221) at the top of the 10-min tutorial that what follows is Python code, and that you can use either a script or interactive session; hopefully this clears up the issue.
_Should be: ax.imshow(default_delta.bed_elevation, vmax=-3)_ Thank you for catching this mistake. We have corrected the variable name in this section.

User guide:

Under “morphodynamics” I find notes that say “incomplete” Thank you for pointing this out, we finished this section of the documentation (here in #221).
Code snipped needs to be mdl = pyDeltaRCM.DeltaModel() as above, no? Also under “updating model boundary conditions” Thanks for catching this. We have corrected the text.
Perhaps the example “configuring an input YAML file” should be the same one used in the next sections? This is a helpful suggestion, thank you. We have changed the file name and changed variables to match those in the next section, hopefully making this simpler for new users (here in #218).
“Starting Model Runs” – clarify that so far the examples have been using the low-level API? When would you want to use one or the other? Thanks for this suggestion. We have added a note to the end of the "Starting model runs" section that describes when you may want to use each API level (here in #218.
Again, this needs to be pyDeltaRCM.DeltaModel(input_file=model_configuration.yml’)`, right? Thanks for catching this. We have corrected the text.
I’m trying the example here but it’s taking ridiculously long. I’m just running it for 5 timesteps which for the single simulations was taking around a minute altogether, but for some reason the model is getting stuck on these simulations. Thanks for identifying this issue. dx: 2.0 was a bad choice on our part for the example; setting this relatively small grid size (default is dx: 50) greatly increases the number of cells in the domain and thus the required computation time. We have changed this value to dx: 40.0, which should run only a little slower than the default.
I tried removing the dx: 2.0 line from the above and got the model to run, but it’s not creating two output folders as it’s supposed to. In the log file, I get (below) Sorry for this error in the log. Actually though, I think the model did work --- are you sure it didn't create the folders and complete the runs? In order to produce this error (and no others), I think it would have had to have run successfully. But, about this error specifically, it's actually not a problem at all. The error indicates that there is no NetCDF output file to close, which is expected since out yaml file did not specify to create any output NetCDF. Nevertheless, it's unhelpful to throw an error that isn't really an error, so we fixed that in #222; the new behavior is to just report to the log that there is no output folder. We also added a small comment to the guide to indicate that there are no outputs for the runs you executed, hopefully making the whole example more clear.
```
2021-07-17 22:17:32,497 - ERROR - Failed to close output NetCDF4 file
2021-07-17 22:17:32,497 - ERROR - 'DeltaModel' object has no attribute 'output_netcdf'
Traceback (most recent call last):
File "C:\Users\salte\anaconda3\envs\deltarcm\lib\site-packages\pyDeltaRCM\model.py", line 234, in finalize
self.output_netcdf.close()
AttributeError: 'DeltaModel' object has no attribute 'output_netcdf'
```
How do I do this (where do I find the model code to copy?) I ended up downloading it from github, but is it on my machine somewhere? If I did want to modify the source code are there instructions for recompiling it? Thanks for raising this point. This questions comes down to general-Python and programming related issues, though, rather than issues with our package . Where the source code for pyDeltaRCM lives on your machine will depend on 1) how you decided to install Python/Anaconda, 2) how you decided to install pyDeltaRCM, and 3) your operating system. We have added a note to this section specifying that developers should go back and install via the developer instructions if they did not. Using the developer installation will allow you to store the source code wherever you want, modify and manage with git, and the modified source will be used whenever the package is imported.
The notes below seem odd. The first note is what I would have expected the model to do. I don’t understand why you say it violates mass conservation; presumably mass in through the inflow equals mass out through the downstream boundary? And why doesn’t the model work if the delta reaches a “grade” condition (and what does it mean physically when the model “reaches grade,” i.e. what does background slope mean?) Thanks for bringing up these notes. We agree that, as written, these notes were confusing. We have rewritten to clarify why models should be stopped before reaching the domain edge, and why we have found it best to stop before reaching the background slope; "grade" was the wrong word choice. You can see the revised text here in #221 or rendered here.
Why is so much extra info provided here about subsidence but not other aspects of the model? Perhaps it would make more sense to place this section under “Examples of using and working with pyDeltaRCM?” We have a lot of documentation about subsidence for the simple reason that we have worked in modifying this aspect of the model the most (e.g., Moodie et al., in review). Eventually, we hope that other model aspects will also be more thoroughly documented, but for now, we just refer the user to the original DeltaRCM references or to the source code. We have made no changes here.
Does the model include stratigraphy capabilities? If so, there should be a section in the user manual explaining how it works and how to run the model with stratigraphy. Thanks for this question. pyDeltaRCM does not explicitly track stratigraphy, but does have an attached array that tracks the composition of the water-sediment surface. If you output model data (bed elevation and sand fraction at a minimum) at a regular interval though, one can use this information to compute stratigraphy after the model run is completed. The documentation describes how to include gridded variables into the output netCDF file, and the YAML configuration page includes these flags. Though stratigraphy is outside the scope of pyDeltaRCM, we are working on making it easier to interrogate pyDeltaRCM outputs, including stratigraphy, with our DeltaMetrics project.
Boundary conditions: how would one go about changing the boundary condition types? E.g. how would I implement a model with two inlets? Or a water/sediment source within the domain? From looking at the source code it looks like I would have to modify the water and sediment routing tools --- could you provide any instructions about how to create and use your own modified version of those tools? Thanks for this question, as it highlights the appeal of pyDeltaRCM! Both boundary condition examples you suggest should be possible, but are not trivial changes, and so they aren't supported "out of the box". With pyDeltaRCM we have produced a tool for scientists that is relatively flexible and extensible (as compared to, e.g., Delft3D), but this means that scientist will need to dig into the code to make modifications. Our goal with our documentation is to provide examples of how to extend and the API documentation shows what to extend, so that a scientist should be able to determine how to implement their own experiment with the documentation and commented source code. For example, a starting point for your questions would be the self.inlet model attribute, which controls where water and sediment enter the domain.
Examples of “modifying internal computations” and “modifying behavior of the preprocessor” are missing. The former seems important and useful to have examples for. I’m not sure what the latter means. Thanks for pointing this out. Similar to the previous question, we don't intend to give examples for every use case, and these are cases we haven't yet explored in our scientific studies. We made the headers and left them empty to give us a reminder to be creative and think about new experiment designs!
Could you add some explanation to the note below? I would think ensuring mass conservation for all sediment classes would be important. This note is correct, and this is the way the DeltaRCM model was originally formulated. The following holds for both sand and mud, but I will use mud as an example. At each mud parcel step, the mud parcel volume may be increased by eroding mud from the bed if the local velocity exceeds the threshold entrainment velocity. There is no check to ensure that there is actually any mud on the bed to erode though. The volume of sediment eroded from the bed is accounted for though, and the bed elevation is lowered accordingly. So, there is total sediment mass conservation during erosion, but not conservation of individual sediment "grain size" classes.
_There’s a “to do” note under “developer guide” and under “inittools” Thank you for pointing this out, we have finished the Developer Guide (here in #221) and the init_tools API page (here in #221).

DeltaRCM / pyDeltaRCM

Salter JOSS Review #217