search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
713 stars 38 forks source link

[REVIEW]: Nyx: A Massively Parallel AMR Code for Computational Cosmology #3068

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @jmsexton03 (Jean M. Sexton) Repository: https://github.com/AMReX-Astro/Nyx Version: 21.07 Editor: @dfm Reviewers: @c-white, @teuben Archive: 10.5281/zenodo.5059767

: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

Status badge code:

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

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

@c-white & @bwoshea, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @dfm 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 @c-white

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @teuben

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

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

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

watching

  1. 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 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/mnras/stz984 is OK
- 10.1088/0004-637x/765/1/39 is OK
- 10.3847/1538-4357/ab4f75 is OK
- 10.21105/joss.01370 is OK
- 10.1088/1742-6596/1225/1/012005 is OK
- 10.1088/0004-637X/715/2/1221 is OK
- 10.1016/0021-9991(84)90143-8 is OK
- 10.1016/0021-9991(90)90233-Q is OK
- 10.1016/j.jcp.2007.07.035 is OK
- 10.1016/j.jpdc.2014.07.003 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=1.13 s (211.5 files/s, 36640.6 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
C++                              84           3952           2607          17841
C/C++ Header                     33           1192           1009           5343
reStructuredText                 28            947            396           2578
CMake                            26            176            128            651
Python                            6            222            249            638
make                             23            217             91            559
Markdown                          6            102              0            460
Fortran 90                        4             93             72            405
TeX                               3             32              5            315
YAML                              8             27             11            295
INI                               1             30              0            288
Bourne Shell                     13             60            132            152
Bourne Again Shell                3             25             67             44
JSON                              1              0              0              3
--------------------------------------------------------------------------------
SUM:                            239           7075           4767          29572
--------------------------------------------------------------------------------

Statistical information for the repository '9bafa86849ecfe4c5b97bb08' was
gathered on 2021/02/26.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Andrew Myers                    40           580           1039            1.03
Ann Almgren                    301         27441          21525           31.17
Arnur Nigmetov                   6           840            285            0.72
Brian Friesen                   54           464            342            0.51
Cyrus Harrison                   1            45            205            0.16
Dmitriy Morozov                  5           144              1            0.09
Frederick Davies                 3           265             35            0.19
Gunther H. Weber                 2           148             42            0.12
Hannah Ross                      6           173            140            0.20
Jean M. Sexton                 497         26958          18071           28.66
Jean Sexton                     81          4655           2089            4.29
Matt Larsen                      1             0              3            0.00
Michael Zingale                  4            98              6            0.07
Michele Rosso                    2             1              3            0.00
Peter McCorquodale               4            35             10            0.03
Weiqun Zhang                    44          1589           2568            2.65
Wolfram Schmidt                 11          2826            906            2.38
Yinghe Lu                        3            41             34            0.05
Zarija                           3            18          11597            7.39
Zarija Lukic                    31         18944            198           12.18
hr203                            1           989            312            0.83
jbb                              6            74             61            0.09
jmsexton03                       1           604             34            0.41
mic84                            3            43             20            0.04
petermcLBL                      27          1673           2133            2.42
vince                           46          2410            621            1.93
zarija                           8          1178           2598            2.40

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Andrew Myers                381           65.7         32.7                3.94
Ann Almgren               13565           49.4          6.3               11.25
Arnur Nigmetov              598           71.2         17.8               11.04
Brian Friesen                24            5.2         41.2                0.00
Cyrus Harrison               39           86.7         11.4               46.15
Dmitriy Morozov             142           98.6         33.2               16.20
Frederick Davies            256           96.6         12.8               10.94
Gunther H. Weber              2            1.4         48.8                0.00
Jean M. Sexton             7070           26.2         10.5               12.11
Michael Zingale              81           82.7         50.1                3.70
Weiqun Zhang                625           39.3         39.7                1.28
Wolfram Schmidt             973           34.4         45.2               14.59
Zarija Lukic               1370            7.2          7.2               10.88
jbb                          31           41.9          6.2               16.13
mic84                        38           88.4          1.4                0.00
petermcLBL                  327           19.5         44.9               13.76
vince                       346           14.4         22.4                4.91
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

c-white commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

c-white commented 3 years ago

@dfm I have finished my review, and everything seems fine to me. Some notes:

  1. Performance: Everything installed (remarkably smoothly!) and I was able to run some basic parallel simulations on a modest cluster. This doesn't cover massive simulations, or GPU clusters for that matter, but I have no reason to believe things won't work in that regime.
  2. Functionality documentation: I don't know if you're looking for documentation of every function in the code base, which there is not, but I don't think that's quite necessary for a code this size. The documentation does thoroughly describe user inputs, as well as the underlying equations and algorithms employed.
  3. Maybe I don't understand whedon, but the pdf it just generated doesn't have the changes the authors recently pushed to the paper.
dfm commented 3 years ago

@c-white: Thanks for this! Your interpretation of the performance and API docs requirements sounds good.

For the paper, I am seeing the most recent changes at the link above: https://github.com/openjournals/joss-reviews/issues/3068#issuecomment-790986670 Are there other commits that are missing? Thanks again!

c-white commented 3 years ago

@dfm I agree the changes are there now, but I swear they weren't earlier. Perhaps it was a caching issue on my end.

whedon commented 3 years ago

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

bwoshea commented 3 years ago

@whedon I'm sorry, I haven't had the chance to start reviewing this yet. It's going to be impossible for me to do it this week, but I will be able to work on it the week of March 22nd.

whedon commented 3 years ago

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

@whedon commands
bwoshea commented 3 years ago

@jmsexton03 apologies for the delay on the review - I'm happy to do it, but it's hitting at a very difficult time of the semester!

dfm commented 3 years ago

@bwoshea: thanks for the update! No stress - let me know if you have any questions as you get started on the review.

dfm commented 3 years ago

:wave: - Hi @bwoshea! Any updates or questions on this? I just wanted to ping to make sure that this stays on your radar.

dfm commented 3 years ago

I have not been able to get a hold of @bwoshea via email either so I'm in the process of recruiting a new reviewer. Thanks all for your patience!

teuben commented 3 years ago

I have another review due this week, if by next weekend you haven't found somebody, i might even enjoy to look at this code

On Sun, Apr 25, 2021, 11:17 Dan Foreman-Mackey @.***> wrote:

I have not been able to get a hold of @bwoshea https://github.com/bwoshea via email either so I'm in the process of recruiting a new reviewer. Thanks all for your patience!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3068#issuecomment-826341114, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ4MGNULX3WKQZ6MKXHYEDTKQXCLANCNFSM4YINZMTQ .

dfm commented 3 years ago

@whedon add @teuben as reviewer

Thanks Peter for agreeing to step in as the second reviewer. I will update the checklist at the top for you momentarily. Please don't hesitate to ask if you have questions as the review progresses. Thanks again!

whedon commented 3 years ago

OK, @teuben is now a reviewer

dfm commented 3 years ago

:wave: @teuben - have you had a chance to get started on this review? Let me know if you have any questions/updates/etc!

teuben commented 3 years ago

yes, i had downloaded the code, and browsed around it's capabilities. I noted (happily) that Nyx is already in ascl on https://ascl.net/1712.006, so it's been around a while. But I also noted it's been 3 weeks, so easy to slip in time. I'll get some hours in this week, it's a fun code.

teuben commented 3 years ago

How strict are we on filenames. If so, the license.txt file should be renamed to LICENSE

dfm commented 3 years ago

Thanks @teuben! license.txt is an acceptable name for the license, although it looks like perhaps the copyright info is a little out of date. It would be fine (encouraged?) to change that to LICENSE.txt or LICENSE to be more consistent with standard practices, but I wouldn't require it!

teuben commented 3 years ago

@jmsexton03 @dfm I have some suggestion for the article. Here or in an issue?

teuben commented 3 years ago

@jmsexton03 @dfm I also went over the install using the documentation. It works, but I find it a bit frustrating that there are two install methods: make and cmake. with make there is no configure, plus the name of the executable (in e.g. the MiniSB example) depends not only on make/cmake, but even on the settings in the GNUMakefile. Thus I could not run an example via copy/paste. Is it really needed that the name of the executable contain all the provenance? Let the user figure out a way if they really want to maintain cross-compile like executables. This would also simplify your documentation :-) Just a thought.

dfm commented 3 years ago

@jmsexton03 @dfm I have some suggestion for the article. Here or in an issue?

@teuben: The standard procedure here would be to open an issue (or several) on AMReX-Astro/Nyx with your comments. It's useful (for me, especially!) if you can reference this issue thread on those issues.

teuben commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

teuben commented 3 years ago

@jmsexton03 @dfm i got a new article proof, but it doesn't show the changes that were done in https://github.com/AMReX-Astro/Nyx/issues/80

dfm commented 3 years ago

@teuben: I'm seeing all the recent changes (e.g. https://github.com/AMReX-Astro/Nyx/commit/e5b9cad659b406fa7671eaf0e1454e36ba06c208) when I click through. Can you point me to a specific change that you're not seeing?

teuben commented 3 years ago

@dfm when i looked at the new paper proof, there is no reference for Strang splitting(3rd par) or the word "integalactic" in the 2nd sentence is misspelled. These were fixed in paper.md, but they didn't show up for me in the paper proof just generated.

True to form, I looked at the PDF generated again, and now it's good.... perhaps I looked too soon. Sorry for the confusion.

c-white commented 3 years ago

@teuben I had the same issue. I'm now 90% sure there's a caching issue on whedon's end. While it might instantly generate a new pdf somewhere, it uses the same link as before, and it takes time for the link to point to the updated document.

teuben commented 3 years ago

@dfm i'm almost done with the review, the issue 81 just submitted should be my last dot on the I.

teuben commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

teuben commented 3 years ago

@dfm now the article proof was correctly updated as it was generated. This also concludes the last comments I had, I have checked off all my review checklist, I'm happy to recommend publication.

jmsexton03 commented 3 years ago

@dfm Should I make a new github tag whenever changes are final? Did whedon automatically check the doi's like it did for the earlier article proofs? I want to make sure the new references are good too.

dfm commented 3 years ago

@teuben: Thanks for the update and for your review!!

@jmsexton03: Give me a day or two to take a final read through and I'll check all those things. After that, I'll have a couple of quick steps (including tagging a release) for you before the final processing. This shouldn't take long - thanks for your patience!

dfm commented 3 years ago

@jmsexton03: I've opened a pull request with some edits to the paper. Can you take a look at that and once you've merged, please take the following steps:

Let me know if you have questions or run into any issues!

dfm commented 3 years ago

@whedon check references

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

OK DOIs

- 10.1137/0705041 is OK
- 10.1093/mnras/stz984 is OK
- 10.1088/0004-637x/765/1/39 is OK
- 10.3847/1538-4357/ab4f75 is OK
- 10.21105/joss.01370 is OK
- 10.1088/1742-6596/1225/1/012005 is OK
- 10.1088/0004-637X/715/2/1221 is OK
- 10.1016/0021-9991(84)90143-8 is OK
- 10.1016/0021-9991(90)90233-Q is OK
- 10.1016/j.jcp.2007.07.035 is OK
- 10.1016/j.jpdc.2014.07.003 is OK
- 10.1088/0067-0049/211/2/19 is OK
- 10.1086/317361 is OK
- 10.1051/0004-6361:20011817 is OK
- 10.1086/313015 is OK
- 10.1145/2929908.2929916 is OK
- 10.3847/1538-4365/ab908c is OK
- 10.1093/mnras/stx1643 is OK
- 10.1093/mnras/stv195 is OK
- 10.3847/1538-4357/abed5a is OK

MISSING DOIs

- None

INVALID DOIs

- None
jmsexton03 commented 3 years ago

I have created the tagged release: https://github.com/AMReX-Astro/Nyx/releases/tag/21.07 I have published a zenodo release associated with that tag: https://zenodo.org/record/5059767

The incremented tag is 21.07 The zenodo DOI for 21.07 is: 10.5281/zenodo.5059767 The zenodo DOI for "All Versions" is 10.5281/zenodo.5059766

dfm commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

dfm commented 3 years ago

Thanks @jmsexton03! Everything is looking good from my end. Can you take one last look at the proofs 👆 and then, once you sign off, we'll be good to go!

jmsexton03 commented 3 years ago

Good to hear @dfm, I rechecked the proofs, and everything looks correct to me (aside from the DRAFT watermark, of course 😉)

dfm commented 3 years ago

@whedon set 21.07 as version

whedon commented 3 years ago

OK. 21.07 is the version.

dfm commented 3 years ago

@whedon set 10.5281/zenodo.5059767 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5059767 is the archive.