[REVIEW]: pyro: a framework for hydrodynamics explorations and prototyping

[whedon]

Submitting author: @harpolea (Alice Harpole)
Repository: https://github.com/python-hydro/pyro2
Version: v.3.1
Editor: @labarba
Reviewer: @mikaem, @ngoldbaum
Archive: 10.5281/zenodo.2575565

Status

Status badge code:

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

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

@mikaem & @ngoldbaum, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @labarba know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @mikaem

Conflict of interest

• As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• I confirm that I read and will adhere to the JOSS 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?
• Version: v.3.1
• Authorship: Has the submitting author (@harpolea) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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

• Authors: Does the paper.md file include a list of authors with their affiliations?
• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @ngoldbaum

Conflict of interest

• As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• I confirm that I read and will adhere to the JOSS 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?
• Version: v.3.1
• Authorship: Has the submitting author (@harpolea) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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

• Authors: Does the paper.md file include a list of authors with their affiliations?
• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

[whedon]

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @mikaem, it looks like you're currently assigned as the reviewer for this paper 🎉.

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

2. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

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

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

👉 Check article proof 📄 👈

[labarba]

👋 @mikaem, @ngoldbaum — Thanks for agreeing to contribute this review! This is where the action happens. You have a reviewer checklist at the top of this issue, and you are welcome to ask any questions or leave comments for the authors here, as well as opening new issues in the submission repository if needed. (If you do, mention the issue in a comment here, to get a cross-link.)

[ngoldbaum]

I've gone through the reviewer checklist and I think this is ready to be published as-is.

My only substantive comment besides the issues and PRs I opened in the pyro2 repository is that the paper would be clearer if the third and second paragraph had their order switched (along with accompanying rewording so it flows well).

[harpolea]

Thanks @ngoldbaum! I agree about the paragraph ordering - I've switched them over in python-hydro/pyro2@0a90bd2

[mikaem]

Hi
Sorry to hold up the review process. I've been familiarising myself with the project, and I started by following the installation instructions on readthedocs. No problem thus far and I get a nice figure from running

./pyro.py advection smooth inputs.smooth

All in all a very nice project:-) I think all there is for me to do really is to tick off boxes and provide just a few advices on improvement (having read the JOSS Guiding priciples: 'a key goal of JOSS is to raise the quality of research software'). And this is not meant as lengthy details of difficulties, just some comments.

I've seen that @ngoldbaum has opened up an issue python-hydro/pyro2#75 regarding installation and I strongly support this suggestion. Furthermore, a good structure with a base solver module and a base problem module would probably make the code easier to follow, and reuse of code would be more intuitive. The softlinked problem folders inside many of the solver folders is probably not the best design;-)

Regarding the documentation, I think a lot of double quotes could be replaced with appropriate cross-links that would make the documentation easier to follow. (For example when the compare.py utility is documented and many others like it.) The API documentation is nicely done (for most) using Numpy style. However, the built API documentation looks a bit strange (see e.g., Parameters here) and I think this is because you are missing 'sphinx.ext.napoleon' from the list of extensions in conf.py. See also here
Also, from the documentation it is not really clear to me how to contribute to this project, only how to get help. May I suggest (as I should probably have done in an issue), that you add a small paragraph around here about how to contribute.

Just a few minor comments to a very nice project. I'll go ahead now and see if I can tick off some boxes:-)

[harpolea]

Thanks for your comments @mikaem!

• Currently simulation_null.py acts as a base for the solvers. As the solvers and problems can be quite different from each other (e.g. compare compressible vs. advection), I'm not really sure that there would be much benefit in having e.g. additional (abstract) base classes for the solvers and problems, and that it may just add an additional level of complexity.
• I agree that more links in the documentation would be helpful - I'll add those in now
• The docstrings in the API are currently processed using the numpydoc extension. The only difference I see between that and sphinx.ext.napoleon seems to be formatting (i.e. the former highlights the variable names, whereas the latter does not)?
• We have a CONTRIBUTING.md file in the main repo, but I agree that this should be added to the documentation as well. I've added that in python-hydro/pyro2@1f14c583

[harpolea]

I added some more links to the documentation in python-hydro/pyro2@47174438. In the example you linked to, compare.py refers to the script itself rather than the module it contains. I think that linking to the module here would not be particularly helpful, as the function of that part of the documentation is to document the use of the script itself.

[mikaem]

Thanks for your comments @mikaem!

• Currently simulation_null.py acts as a base for the solvers. As the solvers and problems can be quite different from each other (e.g. compare compressible vs. advection), I'm not really sure that there would be much benefit in having e.g. additional (abstract) base classes for the solvers and problems, and that it may just add an additional level of complexity.

I understand completely. It's just that when I see a lot of code duplication (or even soft links) I immediately think that this could probably be solved with object orientation and overloading. For example, I could imagine a problem base module where a default init_data method was defined, like the one under advection/problems/smooth.py. Then the problem under advection_rk/problems could simply import that function instead of the soft link. And the problem that was using an advection_fv4 solver could import the base method and overload it appropriately. Works similarly if there was a base problem class with an init_data method to overload. Anyways, how you do it is entirely up to you. I'm not going to fuss over it.

• I agree that more links in the documentation would be helpful - I'll add those in now
• The docstrings in the API are currently processed using the numpydoc extension. The only difference I see between that and sphinx.ext.napoleon seems to be formatting (i.e. the former highlights the variable names, whereas the latter does not)?

Below I add two links to figures that are compiling your documentation with and without sphinx.ext.napoleon. I would say that the second looks much nicer, but it is only formatting, you are right about that:-)

Without sphinx.ext.napoleon, i.e., from readthedocs
With sphinx.ext.napoleon, compiled locally

• We have a CONTRIBUTING.md file in the main repo, but I agree that this should be added to the documentation as well. I've added that in python-hydro/pyro2@1f14c58

Ok, great. In that case I'll tick off my last box and I'm ready to sign off on this:-)

[mikaem]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

👉 Check article proof 📄 👈

[mikaem]

@labarba I recommend this paper for publication!

[labarba]

Minor editorial suggestions:

• Perhaps write pyro in code format? (otherwise, it looks weird in lower case starting a sentence)
• pyro is a python-based >> capitalize Python
• The low Mach number atmospheric solver >> low-Mach-number (hyphenate compound adjective)
  (twice)
• python's pickle() >> capitalize Python
• main python code >> capitalize Python
• python functions >> idem
• entirely in python >> idem
• low Mach number atmospheric solver* >> low-Mach-number
• Also new support >> comma after "Also"

[harpolea]

I've updated the paper file in python-hydro/pyro2@1ab2230a to capitalize the Python's and format the pyro's. In the (astrophysical) literature, it's normally written as low Mach number without the hyphens, so I think it would be better left as it is?

[harpolea]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

👉 Check article proof 📄 👈

[labarba]

Hi @harpolea — Time to make a tagged release of the software (and report the version number here), then an archive in Zenodo or a similar service (and report the DOI here). Cheers!

[harpolea]

Hi @labarba - I've made a tagged release (v. 3.1) and have created a Zenodo DOI (10.5281/zenodo.2575565)

[labarba]

Can you edit the metadata of the Zenodo deposit so it matches the title and author list of the JOSS paper?

[harpolea]

Fixed

[labarba]

@whedon set 10.5281/zenodo.2575565 as archive

[whedon]

OK. 10.5281/zenodo.2575565 is the archive.

[labarba]

@whedon set v.3.1 as version

[whedon]

OK. v.3.1 is the version.

[labarba]

@whedon accept

[whedon]

Attempting dry run of processing paper acceptance...

[whedon]

OK DOIs

- http://doi.org/10.1016/j.ascom.2014.07.003 is OK
- http://doi.org/10.1088/0004-637X/715/2/1221 is OK
- http://doi.org/10.1088/0067-0049/188/2/358 is OK

MISSING DOIs

- None

INVALID DOIs

- None

[whedon]

Check final proof 👉 openjournals/joss-papers#516

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#516, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true

[labarba]

@whedon accept deposit=true

[whedon]

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

[whedon]

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

Here's what you must now do:

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

Any issues? notify your editorial technical team...

[labarba]

Congratulations, @harpolea et al., your paper is published in JOSS!

Many thanks to our reviewers, @mikaem, @ngoldbaum—you make this possible 🙏

[whedon]

🎉🎉🎉 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](http://joss.theoj.org/papers/10.21105/joss.01265/status.svg)](https://doi.org/10.21105/joss.01265)

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

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

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:

• Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
• Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=