[REVIEW]: CASTRO: A Massively Parallel Compressible Astrophysics Simulation Code

whedon commented 4 years ago

Submitting author: @zingale (M. Zingale) Repository: https://github.com/amrex-astro/Castro Version: JOSS_paper Editor: @eloisabentivegna Reviewers: @kegiljarhus, @bonh, @joshia5 Archive: 10.5281/zenodo.4085328

: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

Status badge code:

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

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

@kegiljarhus & @bonh & @joshia5, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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

Review checklist for @kegiljarhus

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) 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.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@zingale) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] 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?

Review checklist for @bonh

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) 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.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@zingale) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[ ] 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?

Review checklist for @joshia5

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) 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.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@zingale) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] 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

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[ ] 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?

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @kegiljarhus, @bonh 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:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

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

notifications

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

whedon commented 4 years ago

PDF failed to compile for issue #2513 with the following error:

pandoc-citeproc: reference ctu not found pandoc-citeproc: reference ppm not found Error producing PDF. ! Undefined control sequence. l.437 ... research. The GPU development of \castro

Looks like we failed to compile the PDF

zingale commented 4 years ago

can we tell whedon to build the PDF off of the development branch? That's where the review should happen, and the refs are fixed there.

zingale commented 4 years ago

@whedon generate pdf from branch development

whedon commented 4 years ago

Attempting PDF compilation from custom branch development. Reticulating splines etc...

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

zingale commented 4 years ago

that proof looks good

eloisabentivegna commented 4 years ago

@whedon generate pdf from branch development

This should do, @zingale.

whedon commented 4 years ago

Attempting PDF compilation from custom branch development
. Reticulating splines etc...

whedon commented 4 years ago

PDF failed to compile for issue #2513 with the following error:

error: pathspec 'development ' did not match any file(s) known to git. error: pathspec 'development ' did not match any file(s) known to git. pandoc-citeproc: reference ctu not found pandoc-citeproc: reference ppm not found Error producing PDF. ! Undefined control sequence. l.437 ... research. The GPU development of \castro

Looks like we failed to compile the PDF

eloisabentivegna commented 4 years ago

@whedon generate pdf from branch development

whedon commented 4 years ago

Attempting PDF compilation from custom branch development. Reticulating splines etc...

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

eloisabentivegna commented 4 years ago

OK, all good!

kegiljarhus commented 4 years ago

Dear @eloisabentivegna and @zingale. This is my first review for JOSS, so please bear with me. I have gone through the review checklist. I had no problems installing the software, running a problem and visualizing the results. Castro is a highly advanced software package which solves relevant scientific problems using state-of-the-art numerical methods. The software seems well-designed and the documentation is also extensive and well-written.

I only have a few minor comments to be addressed:

Some developers listed as core developers in the preface of the documentation are not included as authors: Beckner, Day, Lijewski, Malone, Nonaka. There is also one author, Sexton, not listed as a core developer. Could you please comment on this?
There appears to be some automated testing and regression testing, but it is not clear to me how these are executed as part of the continuous integration or how to run these manually. Could you please make this clearer, preferably in the README?

bonh commented 4 years ago

@eloisabentivegna @zingale Thanks for letting me help in the review. I have already cloned the repo and briefly looked through the awesome software and paper. I will be done with the first round of review (and by my first impression this is the final round too) by Friday afternoon this week.

eloisabentivegna commented 4 years ago

Hi @kegiljarhus, thanks for your review and update!

Could you raise those issues in Castro's repository? This will ensure your suggestions will enter the package's history and be properly credited. You can create a mention here by using this issue's URL in the repository's issue, if you wish (see https://joss.readthedocs.io/en/latest/reviewer_guidelines.html#guiding-principles).

zingale commented 4 years ago

@kegiljarhus thank you for the comments.

For the first, it is indeed the case that those names were listed on the developers page, but none of them have made any contributions in the last 6 years and some of the names don't show in the git history. According to our policy in the README.md, we consider developers as active if they've contributed in the last 3 years. I've moved the inactive names to a "Former Developers" category (through this PR: https://github.com/AMReX-Astro/Castro/pull/1150 ). I've also added Jean Sexton to the list of developers.

For the second, our testing is currently manually triggered on PRs using our regression testing framework here: https://github.com/AMReX-Codes/regression_testing . Part of the reason is because we need to test against 3 different repos (Castro, Microphysics, and AMReX) and we want to make sure that not just Castro changes are clean, but that changes to those other repos also don't affect Castro. Also, our test suite is expensive to run and also uses GPUs, so we use a local machine to do this. For most PRs you'll see a link to the test results. The master list of CPU tests is here: http://groot.astro.sunysb.edu/Castro/test-suite/gfortran/ and for GPUs is here: http://groot.astro.sunysb.edu/Castro/test-suite/gpu/

zingale commented 4 years ago

Note: in addition to being triggered manually on PRs, the test suite also runs nightly using the latest AMReX, Castro, and Microphysics.

kegiljarhus commented 4 years ago

Thank you for the suggestions, @eloisabentivegna. Since @zingale already updated the developer list, and the pull request links to this review, I believe that is sufficient and we do not need to create a new issue. The comments on testing are also satisfactory from my side.

eloisabentivegna commented 4 years ago

@whedon add @joshia5 as reviewer

whedon commented 4 years ago

OK, @joshia5 is now a reviewer

bonh commented 4 years ago

https://github.com/AMReX-Astro/Castro/issues/1154

bonh commented 4 years ago

https://github.com/AMReX-Astro/Castro/issues/1155

bonh commented 4 years ago

@eloisabentivegna On references: "Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)?" As this is not my field of expertise I am not really able to determine if the references concerning the physics and mathematics for this particular problem are complete and/or appropriate.

bonh commented 4 years ago

@eloisabentivegna On Functionality: "Have the functional claims of the software been confirmed?" I do not have a hpc cluster available for testing so I am not really able to confirm the code to be able to conduct "massively parallel [...] simulations"

Same for Performance.

Same for Accuracy.

Same for Installation on a hpc cluster.

bonh commented 4 years ago

@zingale Really impressive work!

joshia5 commented 4 years ago

@whedon generate pdf from branch development

whedon commented 4 years ago

Attempting PDF compilation from custom branch development. Reticulating splines etc...

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

eloisabentivegna commented 4 years ago

@bonh, thanks for your questions about the reference list box -- please feel free to leave it unticked.

Regarding the code tests, we only really expect reviewers to check the small-scale examples (as a rough rule of thumb, those that can be executed on a reasonable laptop). For all the other tests, please simply check that the results appear sensible and generally consistent.

Thanks for all the useful comments!

bonh commented 4 years ago

So I am done :+1:

joshia5 commented 4 years ago

@zingale, @eloisabentivegna following are my comments and questions:

Explicit instructions or commands for loading modules could be given in the installation instructions, I found that python>=3.7 might not be preloaded on all systems
I could not find significant commits in the repo for author John bell.
I could run the test case but is there a specific version requirement on Paraview to visualize the output *plt files? What reader/format should I use while opening the files?
In the paper, comparisons are made with other packages but for the checklist requirement, I cannot comment on whether these other packages are commonly used
While the README mentions how to contribute to the software & Report issues, i could not find guidelines on how a third party can seek support.
As I do not have research expertise in this field, I cannot comment on completeness of references
license: it is mentioned in the commit comment of the license file that castro has gone through paperwork for BSD license, does this mean that the license is a BSD type ?

Thanks, Aditya

zingale commented 3 years ago

Thanks for the comments. I will answer them each inline here.

Can you point out where we suggest python 3.7? It looks like we require 3.6. If you have suggestions for what types of machines you think there should be instructions for, please share. There are so many variations that it would be hard to cover them all.

Python version 3.5 has reached "end of life": https://www.python.org/downloads/release/python-3510/ so we believe requiring 3.6 or later is reasonable.

For working at supercomputing centers, we do provide instructions, linked to from this page of the docs (at the bottom): https://amrex-astro.github.io/Castro/docs/mpi_plus_x.html

zingale commented 3 years ago

Not all Castro developers write code. John contributed the initial version of the hydro solver (before Castro was in version control) and he works on the mathematical aspects of the algorithm development (most recently the SDC stuff).

zingale commented 3 years ago

We don't have experience with Paraview, so we will remove it's mention from the docs. All of the developers use yt.

zingale commented 3 years ago

I am not sure if there is something for us to address here

zingale commented 3 years ago

The last section of the README has a link to the castro-help mailing list. Further, the mailing list is featured under the "get help" button on the main website: https://amrex-astro.github.io/Castro/

zingale commented 3 years ago

We are not sure if there is something for us to address here

zingale commented 3 years ago

The license is OSI approved: https://opensource.org/BSD-3-Clause-LBNL

eloisabentivegna commented 3 years ago

@zingale, thanks for the replies. There is nothing for you to do regarding points 4. and 6. I will now wait for @joshia5 to comment back and share his final recommendation.

joshia5 commented 3 years ago

@zingale. Thanks for your replies. Regarding comment 1, I was on RHEL and Ubuntu systems. @eloisabentivegna looks good to me.

eloisabentivegna commented 3 years ago

Thanks, @joshia5! And thanks again to @kegiljarhus and @bonh for your recommendation. I am pleased to start the pre-publication stage for this submission. 🎉

@zingale, could you please proceed issuing a tagged release and creating the archive? Let me know if you need assistance with these steps.

eloisabentivegna commented 3 years ago

@zingale, did you manage to create the release and the archive? Please let us know if you need assistance.

zingale commented 3 years ago

sorry... I lost track on things for a bit. I'll do this now.

zingale commented 3 years ago

Here's a Zenodo DOI for a tagged release: https://zenodo.org/record/4085328

zingale commented 3 years ago

sorry, that was the zenodo record Here's the DOI: 10.5281/zenodo.4085328

eloisabentivegna commented 3 years ago

@whedon set 10.5281/zenodo.4085328 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4085328 is the archive.

eloisabentivegna commented 3 years ago

@zingale, I am wondering whether the release should have a traditional numerical tag, rather than a special label like "JOSS_paper", so users can readily identify whether a given code version precedes or follows the one published in JOSS.

@arfon, @danielskatz, what do you think?

openjournals / joss-reviews