Submitting author: @elbeejay (Jayaram Hariharan)
Repository: https://github.com/passaH2O/dorado
Version: v2.1.0
Editor: @kbarnhart
Reviewer: @dbuscombe-usgs, @gassmoeller
Archive: 10.5281/zenodo.4118342
: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 badge code:
HTML: <a href="https://joss.theoj.org/papers/f1f61292f998588ae06bb1cd14dd4063"><img src="https://joss.theoj.org/papers/f1f61292f998588ae06bb1cd14dd4063/status.svg"></a>
Markdown: [](https://joss.theoj.org/papers/f1f61292f998588ae06bb1cd14dd4063)
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.)
@dbuscombe-usgs & @gassmoeller, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
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 ✨
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dbuscombe-usgs, @gassmoeller 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:
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
@dbuscombe-usgs and @gassmoeller thanks for agreeing to review this submission to JOSS. As noted above, we are trying to adjust to authors, reviewers, and editors needs under COVID. Thus we are asking reviewers try and complete reviews in 6 weeks. I am going to use whedon the editorial bot to remind you about your review in three weeks, half way through the requested timeline.
If there are any questions from the authors or reviewers throughout the process, please feel free to let me know here or directly at [email protected].
Thanks again for contributing to the JOSS review process.
@whedon remind @dbuscombe-usgs in three weeks
Reminder set for @dbuscombe-usgs in three weeks
@whedon remind @gassmoeller in three weeks
Reminder set for @gassmoeller in three weeks
:wave: @dbuscombe-usgs, please update us on how your review is going.
:wave: @gassmoeller, please update us on how your review is going.
when I'm done with my review, do I just paste it here? I see the 'checklist' but the instructions are vague on the actual review submission process
Ok, I don't see any place else to put this review so here goes!
I confirm that I have read the JOSS conflict of interest 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.
I confirm that I read and will adhere to the JOSS code of conduct.
Paper:
• Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided? Yes
• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is? Yes
• State of the field: Do the authors describe how this software compares to other commonly-used packages? Yes
• Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)? Yes
• 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? Yes
The statement of need should lead with a definition of the term ‘particle tracking’. It is clear to me that this is shorthand for ‘tracking of water – including particles – in a Lagrangian framework’, but it could be clearer to the reader.
I also recommend adding the theory section in the documentation to the paper (https://passah2o.github.io/dorado/background/index.html). It is well written and answers many outstanding questions I have when left with when reading the paper on its own.
Documentation:
• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is? yes
• Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution. Yes
• Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems). Yes
• Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)? Yes
• Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified? Yes
• 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
No. More instruction for how to make a pull request (e.g. https://docs.fast.ai/dev-setup), and how to submit an issue (including what information should be listed when doing so) is required. Also, provide a direct link to the github repo here
What is the motivation for maintaining both pip and conda versions? Seems like picking one and ensuring support would be easier. Also, I suggest you recommend users make use of a virtual environment (venv) or conda environment to make installation, maintenance and compatibility with other projects easier.
The demo on https://passah2o.github.io/dorado/quickstart/index.html#demo-1-using-the-high-level-api is missing:
“import matplotlib.pyplot as plt”
Also, please provide some higher level explanation for what this example is actually doing and what this is a simulation of.
General checks
• Repository: Is the source code for this software available at the repository url? Yes
• License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license? Yes
• Contribution and authorship: Has the submitting author made major contributions to the software? Does the full list of paper authors seem appropriate and complete? Yes
Functionality
• Installation: Does installation proceed as outlined in the documentation? Yes
• Functionality: Have the functional claims of the software been confirmed? Yes
Are there any plans to add or change functionality in the future? If so, a roadmap would be a good addition. It would also be a good way for others could see themselves contributing
Instead of using the future package, presumably to make things backwards-compatible, I would suggest catering ONLY to python 3 and later. This is a new program so backwards-compatibility is certainly not expected and probably not necessary.
Great coding and commenting!
Accept with these minor comments
@dbuscombe-usgs thanks for the review (and sorry to miss this yesterday). You should be able to check the checkboxes... I'll re-invite you just in case you don't have check-box privileges, and if that doesn't work, let me know.
Sorry that exactly how to provide the review was unclear. Pasting your comments in as you have done is just fine. For more substantial comments we recommend that reviewers open an issue in the main repository. I've now copy-pasted your comments into in-repo issues. This just helps keep track of what changes in-repo are associated with the review.
It looks like @elbeejay has already started addressing these issues. 👍
@whedon re-invite @dbuscombe-usgs as reviewer
The reviewer already has a pending invite.
@dbuscombe-usgs please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations
@kbarnhart - I don't know how to check boxes, but I responded 'yes' to all. Am I done?
@dbuscombe-usgs I've gone ahead and checked all the boxes except for community guidelines (where I think you indicated "no"). You should be able to check them with your mouse. Its not critical though, as you indicated your responses in your comment.
Other than affirming that @elbeejay has addressed your comments at the very end of the review processes you are done. Thanks for a speedy and thoughtful review.
Thanks. Happy to review again! Next time I will make issues for substantial comments, and check the boxes
I'm not able to check the boxes, on my Brave browser on linux
@arfon FYI. If we expect github md checklists to be checkable on Linux + Brave we have a report that they are not check-able. Obviously a minor point, but a datapoint nonetheless.
Yes we have gotten a bit of a start on addressing those comments. Thanks for the review @dbuscombe-usgs - will let you know once we've addressed everything.
I have reviewed the software and opened a few issues for minor points as linked above. I have some concerns about the code structure that I raise below, but the submission itself was well prepared and the code is easy to read and seems well tested. The software is useful and the paper is a nice summary and fits into JOSS's scope. I would like to have a conversation about the points I raise below, and which ones you can easily address and which have to be left for future work, but after addressing the main points I am open to recommending this submission for publication.
You provide a clear statement of need, but I would prefer if you could specify the target audience a bit more clearly. It seems you are aiming at small- to medium-scale particle models and rapid development and use rather than high-performance and massively parallel models, is that correct? If so, please state.
Your main class that handles all particles is called "Particle". When I looked into the code I expected this to be a class for one single particle. In terms of nomenclature, maybe something like ParticleHandler, or ParticleManager seems more appropriate (I do not insist on those names, I only suggested them, because in my own particle implementation I chose the name ParticleHandler). At least I would consider using "Particles" to make clear that it is a class that holds more than one particle. I realize this may be a disruptive change, and I do not know how much backward compatibility you have to consider, but I would like to hear your opinion on the topic.
Your current code works for your purpose and the examples you show, but I am slightly concerned about the future maintainability of the code, because you have mixed responsibilities and an unclear class hierarchy for some of your functions. I will give some examples for your main functions and why that could become a problem later below. I would not consider this a roadblock for the publication in JOSS, but I would like to see as much as reasonable of this fixed at some point and I would like to hear your opinion on the matter:
You define particle locations either based on an initial seed location, or based on existing particle locations either from a previous run, or created by the user. But the initial seed location is expected to be handed over during construction of Particle, while existing locations are handed over to run_iteration. This is a bit confusing for a newcomer, but more importantly: It shows that you are not sure who (which function) is responsible for the particle generation. I would suggest to have one function specific for particle generation, e.g. generate_particles(x_seed, y_seed, start_xindices, start_yindices)
. In a previous particle system I have written we separated that into a set of different functions (Particles::Generators::regular_reference_locations, Particles::Generators::probabilistic_locations, ..., see here if you are interested in details), but that would be overkill for your application here. Still you want to keep that option instead of spreading the responsibility for generating over multiple functions or making your run_iteration
function into a somehow_generate_particles_and_run_iteration
function. This would become a problem once you start to add more particle generation options (e.g. probabilistic particle distribution with varying density functions, particle patterns like discs/circles/lines).
A related comment about assigning responsibilities: I do not understand why the run_iteration
function is a member of Particle
, but the single_iteration
function is a member of Tools
. single_iteration
does the core work of moving the particles, and so I would suspect it to be a member of Particle
. It is true that single_iteration
uses many of the Tools
functions, but that does not mean it is conceptually a tool itself, and I would move it to Particle
. On the other hand the functions coord2ind
, ind2coord
, and unstruc2grid
are typical tools functions and I would expect them in the Tools
class, and not as stand-alone functions in particle_track.py
. More about Tools
below.
Currently Tools
defines a number of utility functions for the Particle
class, and then Particle
is derived from Tools
. But this is not a typical is-a
parent-child relation. Particle
is not a Tools
. Particle
uses Tools
, so instead it should import Tools
, or be given an object of type Tools
upon creation or similar. This is already a problem in your code, because functions in Tools
use member variables that are only read in and initialized in Particle
(e.g. self.velocity in Tools.calc_travel_times). In other words Tools
can not be used without Particle
or used as a base class for anything else. Utility functions or objects should usually not own any member variables (and in particular not rely on member variables of derived classes), and instead be handed all necessary parameters to compute the little computation they are supposed to do.
Related to the last comment: Your documentation in Tools
currently states that Particle
is derived from Tools
and uses its functions, but if that would change in the future you would likely only update the Particle
class and forget to change the documentation of Tools
(not implying you are not careful, this is just what experience in programming shows). Instead this documentation should be in the Particle
class, explaining why Particle
is derived from Tools
, this way if the relationship changes you also update the documentation. Documentation should always state what this thing does, not what others do with it.
@gassmoeller thanks so much for your thoughtful review. 👍
A quick note to @elbeejay and co-authors. As you address these issues I'd recommend you start an in-repo issue, and corresponding with @gassmoeller about the details there. That way the multiple comments can be addressed on separate threads.
@elbeejay it looks like you and co-authors are making great progress addressing the reviewer comments in the source repository. I wanted to check in and see if you had any questions for me.
At this stage of the JOSS review process, we (editor, reviewers) typically just wait for the submitting authors to address any outstanding issues and provided feedback as needed. It looks like you and @gassmoeller are already having this sort of discussion in your in-repo issues. 🎉
There is no standard timeframe for this stage of the review process and we typically don't "pause" the JOSS review (to answer the question raised here.
Thanks for reaching out @kbarnhart. We've been putting together a PR that should address the bulk of the review comments, just double and triple checking it on our end before we merge and respond to the various issues that were opened.
Sounds good @elbeejay. If things arise where I can provide guidance and or support, please let me know.
@kbarnhart I don't know where we'd update this, but some of the changes that have been made during the review process were 'breaking' changes. So in light of that, the code associated with the JOSS paper will be v2.0.0 (or maybe v2.0.x if something small has to change) instead of v1.0.0. Is there an easy way to change this? Thanks!
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@elbeejay not a problem. One of the final things I'll ask you for at the end of the review process is to let me know the version number. I can then update that metadata and the final paper will reflect the correct version.
@gassmoeller and @dbuscombe-usgs, we've done our best to incorporate the changes you both suggested we make to improve the quality of the paper and the code. Thank you both for the thoughtful feedback.
I've tried to address the comments directly in each issue, but didn't want to keep @-tagging you. The bulk of the changes associated with this review were made in PR 15 which you can view if you prefer to see the code "diffs".
For the edits and revisions made to the paper, a new rendered version is available in the whedon post above.
Again, thanks to you both, feel free to comment here or in a specific issue if you feel we've not sufficiently addressed your original comment or if something is unclear.
@elbeejay thanks for the update and for responding to reviewer comments with changes in your submitted software.
@gassmoeller and @dbuscombe-usgs please let me know if you seen any outstanding issues or if there are remaining items that @elbeejay and co-authors should attend to.
@dbuscombe-usgs there is one check-box remaining (related to community guidelines). Since I know you are not able to check the boxes, please let me know if you think it can be checked, and I can edit it. As best I can tell the authors have addressed these concerns.
Thanks all for your contributions to this review process.
I've read over the responses and proof and I am satisfied. Good job authors! Please check my remaining box! (I don't know what is up with that - I also tried on firefox)
I also looked through the changes and the new version of the paper and my comments have been addressed. This is ready for publication from my point of view. :+1:
Thanks again @dbuscombe-usgs and @gassmoeller for taking the time to review this!
@elbeejay FYI the next step is that I do a final copyedit of the paper and let you know if there are any necessary changes. Then we move to the final stage of the JOSS process.
@whedon check references
@whedon generate pdf
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.5194/gmd-11-1405-2018 is OK
- 10.1038/072342a0 is OK
- 10.5194/esurf-3-67-2015 is OK
- 10.5194/esurf-3-87-2015 is OK
- 10.1016/j.ocemod.2014.02.007 is OK
- 10.1525/bio.2009.59.6.8 is OK
- 10.1017/S0022112089002697 is OK
- 10.1002/2017WR021289 is OK
- 10.1029/2018GL079405 is OK
- 10.1016/j.marchem.2016.05.011 is OK
- 10.1016/j.ecss.2016.02.001 is OK
- 10.1016/j.jhydrol.2005.11.055 is OK
- 10.1029/2018WR023527 is OK
- 10.1029/2012GL053476 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@whedon generate pdf
PDF failed to compile for issue #2585 with the following error:
Cloning into '9cca395e6a14bcd2e567e271'...
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@elbeejay and co-authors, congratulations on making it to the end of the JOSS review process. I think the software you submitted was already in great shape, but through the thoughtful comments of @dbuscombe-usgs and @gassmoeller it has been made even better. I want to thank all authors and reviewers for your contribution to this review process.
At this point @elbeejay could you:
I can then move forward with accepting the submission.
Thanks for all of your help through this process @kbarnhart. I think I've managed to do the items listed above.
@whedon set v2.1.0 as version
OK. v2.1.0 is the version.
@whedon set 10.5281/zenodo.4118342 as archive
OK. 10.5281/zenodo.4118342 is the archive.
@whedon accept
Attempting dry run of processing paper acceptance...
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.5194/gmd-11-1405-2018 is OK
- 10.1038/072342a0 is OK
- 10.5194/esurf-3-67-2015 is OK
- 10.5194/esurf-3-87-2015 is OK
- 10.1016/j.ocemod.2014.02.007 is OK
- 10.1525/bio.2009.59.6.8 is OK
- 10.1017/S0022112089002697 is OK
- 10.1201/9781420012514 is OK
- 10.1002/2017WR021289 is OK
- 10.1029/2018GL079405 is OK
- 10.1016/j.marchem.2016.05.011 is OK
- 10.1016/j.ecss.2016.02.001 is OK
- 10.1016/j.jhydrol.2005.11.055 is OK
- 10.1029/2018WR023527 is OK
- 10.1029/2012GL053476 is OK
MISSING DOIs
- None
INVALID DOIs
- None
: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/1842
If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1842, then you can now move forward with accepting the submission by compiling again with the flag deposit=true
e.g.
@whedon accept deposit=true
@elbeejay this is now in the hands of the JOSS handling Editor in Chief who manages final publication.
Great, thanks for all of the help @kbarnhart, this was a pleasant process.
@whedon accept deposit=true
Looks good to me!
Doing it live! Attempting automated processing of paper acceptance...
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨
Here's what you must now do:
Party like you just published a paper! 🎉🌈🦄💃👻🤘
Any issues? Notify your editorial technical team...
Congrats @elbeejay on your article's publication in JOSS!
Many thanks to @dbuscombe-usgs and @gassmoeller for reviewing this, and @kbarnhart for editing.
:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:
If you would like to include a link to your paper from your README use the following code snippets:
Markdown:
[](https://doi.org/10.21105/joss.02585)
HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.02585">
<img src="https://joss.theoj.org/papers/10.21105/joss.02585/status.svg" alt="DOI badge" >
</a>
reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.02585/status.svg
:target: https://doi.org/10.21105/joss.02585
This is how it will look in your documentation:
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: