joss-reviews icon indicating copy to clipboard operation
joss-reviews copied to clipboard

[REVIEW]: NuclearToolkit.jl: A Julia package for nuclear structure calculations

Open editorialbot opened this issue 1 year ago • 32 comments

Submitting author: @SotaYoshida (Sota Yoshida) Repository: https://github.com/SotaYoshida/NuclearToolkit.jl Branch with paper.md (empty if default branch): main Version: v0.1.0 Editor: @rkurchin Reviewers: @mdavezac, @villaa Archive: Pending

Status

status

Status badge code:

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

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

@mdavezac & @villaa, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

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

Checklists

📝 Checklist for @mdavezac

📝 Checklist for @villaa

editorialbot avatar Aug 22 '22 00:08 editorialbot

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf

editorialbot avatar Aug 22 '22 00:08 editorialbot

Software report:

github.com/AlDanial/cloc v 1.88  T=0.14 s (365.9 files/s, 178132.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           34            810            707          22574
TeX                              1             10              0            218
Markdown                         8             54              0            198
TOML                             5             35              2            133
YAML                             3              0              2             84
-------------------------------------------------------------------------------
SUM:                            51            909            711          23207
-------------------------------------------------------------------------------


gitinspector failed to run statistical information for the repository

editorialbot avatar Aug 22 '22 00:08 editorialbot

Wordcount for paper.md is 448

editorialbot avatar Aug 22 '22 00:08 editorialbot

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- h10.1016/S0146-6410(01)00159-4 is OK
- 10.1103/RevModPhys.77.427 is OK
- 10.1103/RevModPhys.92.015002 is OK
- 10.1146/annurev-nucl-101917-021120 is OK
- 10.1016/j.physrep.2011.02.001 is OK
- 10.1088/0031-8949/91/8/083007 is OK
- 10.1103/RevModPhys.81.1773 is OK
- 10.3389/fphy.2020.00098 is OK
- 10.1140/epja/i2003-10096-0 is OK
- 10.1093/ptep/ptac057 is OK
- 10.1017/CBO9780511596834 is OK
- 10.1016/j.cpc.2013.07.022 is OK
- 10.1016/j.nds.2014.07.022 is OK
- 10.1016/j.cpc.2019.06.011 is OK

MISSING DOIs

- None

INVALID DOIs

- None

editorialbot avatar Aug 22 '22 00:08 editorialbot

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

editorialbot avatar Aug 22 '22 00:08 editorialbot

@rkurchin Thank you for your kind help and editorial work. I have a question: If a reviewer points out a bug or typo in the code, is it acceptable to update the code during the review process and should it be tagged?

SotaYoshida avatar Aug 22 '22 00:08 SotaYoshida

@SotaYoshida it is not only acceptable, it is highly encouraged! As for tagging, we will ask you to issue a new release tag at the end of the review process. If you want to do so along the way, that's fine, but it's not necessary.

rkurchin avatar Aug 22 '22 18:08 rkurchin

I got it. Thanks!

SotaYoshida avatar Aug 25 '22 02:08 SotaYoshida

Review checklist for @mdavezac

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

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/SotaYoshida/NuclearToolkit.jl?
  • [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 (@SotaYoshida) 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
  • [x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • [x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • [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?

mdavezac avatar Aug 28 '22 08:08 mdavezac

The package is a truly ambitious effort to translate a number of nuclear structure many-body methods to Julia, with the goal of providing a modular framework to advance such methods for students and researchers. Unfortunately, the code makes little use of best practices in software design (unit-testing, separation of concerns, encapsulation, etc), Julia's capabilities promoting modularity (multiple dispatch), and reads more like a direct translation of an old Fortran code than a modern implementation. This in itself should not stop publication in JOSS. However, I believe the state of the documentation and the testing is not yet sufficient to verify the functionality claims:

  1. there is no example or test for MPI (the code claims to work on supercomputers)
  2. the integration tests such as they are only check the code runs to completion. They do not check the code computes anything, much less that it computes anything correctly.
  3. from the input, it seems the code generates a binary output file. As far as I can tell, there is no documentation of what it contains or how to read and use the data therein.
  4. the API documentation is too often sparse, especially when one considers some functions listed there take 10-20 arguments with no obvious typing. Since this package is meant as a toolkit for the community, I would recommend (though not require) investing some time into it. In this respect, unit-tests demonstrating usage and expected outputs would have been valuable.

Also, the code will not run as a package (see SotaYoshida/NuclearToolkit.jl#27).

There are a few typos in the paper:

  1. first sentence, "among nucleons, nuclear force" -> "among nucleons, [the] nuclear force"
  2. second sentence, "especial[l]y"

mdavezac avatar Aug 28 '22 14:08 mdavezac

@mdavezac Thank you so much for reviewing and feedbacks. I am now dealing with your points and documents, test codes, etc. It may take at least a couple of weeks.

I will make changes in between, but I will ask you to confirm at some point not to bother you repeatedly, so please do not care about the details of the changes until then.

Again, I appreciate you taking the time to review this package.

SotaYoshida avatar Aug 31 '22 04:08 SotaYoshida

@SotaYoshida, just wanted to check in on the progress on these updates. How are things going?

rkurchin avatar Sep 22 '22 21:09 rkurchin

@rkurchin Thank you for the reminder. According to the reviewer(@mdavezac)'s comment, I have made dozens of modifications so far. I will tag v0.1.5 within a couple of days and, then reply to the comments with summary of modifications.

SotaYoshida avatar Sep 23 '22 00:09 SotaYoshida

@editorialbot set v2.0.1 as version

SotaYoshida avatar Sep 26 '22 04:09 SotaYoshida

I'm sorry @SotaYoshida, I'm afraid I can't do that. That's something only editors are allowed to do.

editorialbot avatar Sep 26 '22 04:09 editorialbot

@mdavezac Apologies for the delay.

I have made dozens of modifications on the package according to your valuable comment, and then tagged the latest one as v2.0.1. (For the above command , I didn't understand how to interact with the bot. Please do not care.)

Here is the summary and reply to your comment:

  1. For the documantation

    I have made new pages, HowToUse and FileFormat to show example usage of the main APIs and to explain what input/output files are, on the document.

  2. For test codes

    there is no example or test for MPI (the code claims to work on supercomputers)

    Absolutely. An example code and a test are added to the document and CI test. I am not fully aware of how much is required in terms of unit testing, but I have added a test code to validate the outputs of many-body methods with a specific parameter set. I think this somewhat improved the mitigation of destructive changes during the code development.

  3. the API documentation is too often sparse, especially when one considers some functions listed there take 10-20 arguments with no obvious typing. Since this package is meant as a toolkit for the community, I would recommend (though not require) investing some time into it. In this respect, unit-tests demonstrating usage and expected outputs would have been valuable.

    Yes. First, I have modified main APIs, especially make_chiEFTint(), and structure of functions therein, and the package is now available to run outside the repository. #27 Second, I have added detailed docstrings on the main APIs and homemade (mutable) structs, and added annotations on the structs so that a user can trace the workflow of the code by e.g. GUI editor like VScode.

  4. I fixed the typos in the paper

I know you are very busy, but I hope you could give us another review. Thank you so much for your time.

SotaYoshida avatar Sep 26 '22 05:09 SotaYoshida

@SotaYoshida thanks for all your work on these changes! Just so you know, adding the version tag via the bot is something only I as the editor have the permission to do. I usually wait until the end of the review to do that so it can just be set when it's finalized. So don't worry about it!

rkurchin avatar Sep 26 '22 13:09 rkurchin

Review checklist for @villaa

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

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/SotaYoshida/NuclearToolkit.jl?
  • [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 (@SotaYoshida) 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
  • [ ] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • [ ] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • [x] Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

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

  • [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: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • [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?

villaa avatar Oct 02 '22 16:10 villaa

@SotaYoshida I started reviewing, I think the paper is well written I submitted one issue on GitHub relating to reference links and DOIs for arXiv-only references. I generally tag the issues related to my review with [JOSS Review].

I will continue reviewing the software!

villaa avatar Oct 02 '22 18:10 villaa

@SotaYoshida, thank-you for all your work. I'm afraid I have not been able to get to it yet, but I hope to finish the review by next week.

mdavezac avatar Oct 03 '22 10:10 mdavezac

@editorialbot generate pdf

SotaYoshida avatar Oct 04 '22 00:10 SotaYoshida

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

editorialbot avatar Oct 04 '22 00:10 editorialbot

@editorialbot generate pdf from branch main

SotaYoshida avatar Oct 04 '22 00:10 SotaYoshida

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

@editorialbot commands

editorialbot avatar Oct 04 '22 00:10 editorialbot

@editorialbot set main as branch

SotaYoshida avatar Oct 04 '22 00:10 SotaYoshida

Done! branch is now main

editorialbot avatar Oct 04 '22 00:10 editorialbot

@editorialbot generate pdf

SotaYoshida avatar Oct 04 '22 02:10 SotaYoshida

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

editorialbot avatar Oct 04 '22 02:10 editorialbot

Sorry for the repetition of the commands. I finally found out that how to generate the pdf with GitHub Actions so I will use it...

SotaYoshida avatar Oct 04 '22 02:10 SotaYoshida

(No worries! I cleaned up the comment history a bit for readability 😉)

rkurchin avatar Oct 05 '22 17:10 rkurchin