search

openjournals / joss-reviews

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

[REVIEW]: 𝚗𝚛𝙲𝚊𝚜𝚌𝚊𝚍𝚎𝚂𝚒𝚖 -- A simulation tool for nuclear recoil cascades resulting from neutron capture #3993

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@villaa<!--end-author-handle-- (Anthony Villano) Repository: https://github.com/villano-lab/nrCascadeSim Branch with paper.md (empty if default branch): Version: v1.4.0 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @matthewfeickert, @altavir Archive: 10.5281/zenodo.6056681

: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/d69ced49c5c17fdbf637e0747d815deb"><img src="https://joss.theoj.org/papers/d69ced49c5c17fdbf637e0747d815deb/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/d69ced49c5c17fdbf637e0747d815deb/status.svg)](https://joss.theoj.org/papers/d69ced49c5c17fdbf637e0747d815deb)

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

@matthewfeickert & @altavir, 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 @lucydot 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 @matthewfeickert

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @altavir

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @matthewfeickert, @altavir 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 2 years ago

Wordcount for paper.md is 1201

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

OK DOIs

- 10.1515/iupac.70.0026 is OK
- 10.1103/PhysRevD.91.083509 is OK
- 10.1103/physrevc.43.1086 is OK
- 10.1103/PhysRevC.46.972 is OK
- 10.1016/S0168-9002(03)01368-8 is OK
- 10.1088/1748-0221/16/07/p07032 is OK
- 10.1103/PhysRevC.82.054616 is OK

MISSING DOIs

- None

INVALID DOIs

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

github.com/AlDanial/cloc v 1.88  T=0.18 s (591.1 files/s, 163342.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                       9           2404           2458           9198
C                               13            711            970           2536
SVG                              2              0             21           2248
CSS                              5            334             50           1293
C++                             11            254            252            963
reStructuredText                12            249            337            530
Python                           8            164            269            528
Markdown                        14            189              0            517
C/C++ Header                    10            154            308            484
HTML                             4            131              1            269
Jupyter Notebook                 2              0            508            133
TeX                              1             10              1            125
YAML                             6             16             38            109
make                             3             31             18             84
Bourne Shell                     4             13              6             65
DOS Batch                        1              8              1             26
-------------------------------------------------------------------------------
SUM:                           105           4668           5238          19108
-------------------------------------------------------------------------------

Statistical information for the repository 'e6613cefdb2052624e444cb9' was
gathered on 2021/12/14.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Anthony Villano                 11         21540            431           90.36
Kitty                           11          1089           1254            9.64

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
Anthony Villano           19984           92.8          0.3               20.39
Kitty                       960           88.2          8.6               25.73
whedon commented 2 years ago

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

matthewfeickert commented 2 years ago

@lucydot @altavir As I already have done a few reviews of the project, and @villaa and the rest of the dev team are working on Issues that I raised in the pre review RE: release v1.2.3 (c.f. https://github.com/openjournals/joss-reviews/issues/3932#issuecomment-993030180) I'm going to hold off on starting my review until they finish those and release v1.2.4 and notify us here :+1: (once that is done can we also get whedon to update the Version number in the Issue body?). I suspect that this will only take a few days as the team seemed quite eager to get the review started, which is great.

I'll of course take a look at the paper itself now, but my memory from the previous review was that it was basically fine, so I'm not expecting to have many, if any, comments there.

matthewfeickert commented 2 years ago

@whedon commands

whedon commented 2 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository
villaa commented 2 years ago

@matthewfeickert @altavir we have released v1.2.4 which takes care of (what should have been) an obvious bug identified by @matthewfeickert while running the realizeCascades executable. This should allow better testing for now. The team is also working on the other major thing revealed in the. pre-review, which is related to issue 27 in nrCascadeSim. When that is complete we expect to go to v1.3.0 and I will notify here.

lucydot commented 2 years ago

That's excellent thanks @villaa and @matthewfeickert for the updates.

@altavir I can see you have also started working through the reviewer checklist which is great. I guess you've probably already seen the link to the previous review of this code, but just in case you missed it - it is here: https://github.com/openjournals/joss-reviews/issues/3174.

altavir commented 2 years ago

@lucydot thanks. I will get back to that as soon as I finish some urgent tasks.

lucydot commented 2 years ago

@altavir @villaa @matthewfeickert I am setting my out of office for the next two week Christmas / New Year period. I will check back in next week to see progress, but a heads up that I may not respond to messages within the typical 3 day period.

RE: updating the software version number for review. I can manually update the version number in the review thread at the top, but first will double check with other members of the editorial team that this won't have have any unwanted side effects. The latest software release for review is v1.2.4 with an update to v.1.3.0 expected (as outlined by @villaa in the post above).

lucydot commented 2 years ago

/ooo December 20 until January 3

villaa commented 2 years ago

@altavir @matthewfeickert @lucydot @gerudo7 -- the version v1.3.0 has now been released. This is the best version for your further reviews and comments. There are some issues left from the pre-review but they were both explicitly said to be not required for the review (adding code coverage and using doxygen to auto-generate docs).

We have upgraded so that OSX 10.14, 10.15 are now working on Travis-CI.

We will work on the remaining issues concurrently with the other issues from the review and update here for any changes. I invite you to please give feedback for your review and thank you for agreeing to participate.

whedon commented 2 years ago

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

whedon commented 2 years ago

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

villaa commented 2 years ago

@lucydot @matthewfeickert @altavir -- one last update. We have released v1.3.1 and all issues from the previous review and pre-review are addressed. We have test coverage of around 80%.

If you have not started the review this is the best version to use. If you started on v1.3.0 the only differences are upgraded documentation and code test coverage.

lucydot commented 2 years ago

Thanks for the update @villaa and Happy New Year ✨

I'll update the version number now. Usually we update the version number at the end of the review process - in this case, I'll update now to avoid any confusion as to which version is under review.

lucydot commented 2 years ago

@whedon set version as v1.3.1

whedon commented 2 years ago

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

@whedon commands
lucydot commented 2 years ago

@whedon set v1.3.1 as version

whedon commented 2 years ago

OK. v1.3.1 is the version.

altavir commented 2 years ago

Happy new year to everyone. I've finally found some time to review the code and the paper and open some issues. Not all of them are critical though. What I think is the most important is this one. The lack of comparison of results with other tools as well as regression tests could be a real problem in the future.

I also failed to build the project on WSL even with CERN ROOT installed ( I have no clue why it is needed anyway), so I can't comment on the program working (I will try later to run it again), but even from looking into the code there seems to some things that could be improved. I am not a C++ expert though.

villaa commented 2 years ago

@altavir Happy New Year! Thank you very much for your review, I see the issues on our issue tracker and the team will address them and comment here when they're finished.

Do you know what style of Linux WSL uses? We haven't tested on Windows but could try to match the system.

@gerudo7 do you run WSL?

nuclearGoblin commented 2 years ago

@villaa:

@gerudo7 do you run WSL?

Yes! I've been doing all my work with nrCascadeSim in WSL. I know for certain that it works for me with root version 6.22/00 installed via conda, but I just tried it with the latest precompiled binary for version 6.24 and nrCascadeSim won't make. I'm guessing that the problem is with using the precompiled binaries in WSL, but I'm still testing that theory. (I will work on adding whatever I find to the docs as well.)

altavir commented 2 years ago

I am using Root 6.24.0 installed via nix-env. I think it is better to continue the discussion in your repository, You can open an issue and we can discuss it there.

matthewfeickert commented 2 years ago

Just popping in to say that I'm back from holidays now and so will try to get my review in before Wednesday. I see that the authors have done a lot of nice work, which is great to see, and I look forward to getting to review it. :slightly_smiling_face:

villaa commented 2 years ago

Just popping in to say that I'm back from holidays now and so will try to get my review in before Wednesday. I see that the authors have done a lot of nice work, which is great to see, and I look forward to getting to review it. 🙂

@matthewfeickert Do you have an update?

matthewfeickert commented 2 years ago

Do you have an update?

No. Maybe tonight. High probability before end of day Monday (2022-01-17).

Edit: I've finished my review. I will clean it up for clarity and post it before end of day on Monday (2022-01-17), but my comments are minor as the authors have done excellent work since the pre-review in improving the software.

matthewfeickert commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

altavir commented 2 years ago

The primary critical point that should be fixed in my opinion is https://github.com/villano-lab/nrCascadeSim/issues/75. The part with comparison. Data tests would be welcome for future development, but not so critical.

villaa commented 2 years ago

The primary critical point that should be fixed in my opinion is villano-lab/nrCascadeSim#75. The part with comparison. Data tests would be welcome for future development, but not so critical.

@altavir thanks, we are working on using Geant4 model for our comparison. It is taking a bit of time b/c we want to get the most recent version and I haven't run the module in a while. Incidentally, here is the code we're using for that.

matthewfeickert commented 2 years ago

Thanks for your submission and software @villaa. The following is my review of it, which goes into detail (and so isn't just noted in the first post of this Issue) only to try to help make requested revisions more actionable and clear. As always, the review is following the JOSS paper review criteria. Also, please feel free to ask for clarification on anything as I'd like to make sure that this information is helpful and actionable to you. :+1:

For consistency and reproducibility of all my comments, I will be using the public Docker image atlasamglab/stats-base:root6.24.06 (where ROOT is built from source and installed from the Git tag 6.24.06) to conduct my review which has the following software specs

$ docker run --rm -ti -v $PWD:$PWD -w $PWD atlasamglab/stats-base:root6.24.06
# gcc --version | head -n 1
gcc (Debian 10.2.1-6) 10.2.1 20210110
# root --version
ROOT Version: 6.24/06
Built for linuxx8664gcc on Jan 12 2022, 09:39:00
From tags/v6-24-06@v6-24-06
# root-config --cflags
-pthread -std=c++17 -m64 -I/usr/local/venv/include

Review checklist

This review is for tag v1.3.1 of the project.

Conflict of interest

Code of Conduct

General checks

JOSS asks for a minimum of 3 months of work equivalent effort for the software to be considered. The project definitely fulfills this requirement.

Functionality

Install example: ```console # git clone --depth 1 https://github.com/villano-lab/nrCascadeSim.git --branch v1.3.1 --single-branch # cd nrCascadeSim/ # make g++ -fPIC -c -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc src//isotope_info.c `root-config --cflags` -L. -L/home/data/nrCascadeSim/bin/lib -fprofile-arcs -ftest-coverage mv isotope_info.o /home/data/nrCascadeSim/bin/lib/ g++ -fPIC -c -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc src//weisskopf.c `root-config --cflags` -L. -L/home/data/nrCascadeSim/bin/lib -fprofile-arcs -ftest-coverage mv weisskopf.o /home/data/nrCascadeSim/bin/lib/ g++ -fPIC -c -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc src//lindhard.c `root-config --cflags` -L. -L/home/data/nrCascadeSim/bin/lib -fprofile-arcs -ftest-coverage mv lindhard.o /home/data/nrCascadeSim/bin/lib/ g++ -fPIC -c -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc src//cascadeProd.c `root-config --cflags` -L. -L/home/data/nrCascadeSim/bin/lib -fprofile-arcs -ftest-coverage mv cascadeProd.o /home/data/nrCascadeSim/bin/lib/ g++ -fPIC -c -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc src//edepmath.c `root-config --cflags` -L. -L/home/data/nrCascadeSim/bin/lib -fprofile-arcs -ftest-coverage mv edepmath.o /home/data/nrCascadeSim/bin/lib/ g++ -fPIC -c -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc src//rootUtil.c `root-config --cflags` -L. -L/home/data/nrCascadeSim/bin/lib -fprofile-arcs -ftest-coverage mv rootUtil.o /home/data/nrCascadeSim/bin/lib/ g++ -fPIC -shared /home/data/nrCascadeSim/bin/lib/lindhard.o /home/data/nrCascadeSim/bin/lib/weisskopf.o /home/data/nrCascadeSim/bin/lib/isotope_info.o /home/data/nrCascadeSim/bin/lib/cascadeProd.o /home/data/nrCascadeSim/bin/lib/edepmath.o /home/data/nrCascadeSim/bin/lib/rootUtil.o `root-config --cflags` -L`root-config --libdir` -lCore -lRIO -lTree -o /home/data/nrCascadeSim/bin/lib/libncap.so -fprofile-arcs -ftest-coverage g++ -fPIC -Wl,-rpath,`root-config --libdir`,-rpath /home/data/nrCascadeSim/bin/lib -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc -L. -L/home/data/nrCascadeSim/bin/lib /home/data/nrCascadeSim/bin/realizeCascades.cpp `root-config --cflags` -L`root-config --libdir` -lCore -lRIO -lTree -lncap -o /home/data/nrCascadeSim/bin/realizeCascades -coverage -fprofile-arcs -ftest-coverage g++ -fPIC -Wl,-rpath /home/data/nrCascadeSim/bin/lib -D__GIT_VERSION=\"v1.3.1\" -IMersenne -Iinc -L. -L/home/data/nrCascadeSim/bin/lib /home/data/nrCascadeSim/bin/regexPlayground.cpp -o /home/data/nrCascadeSim/bin/regexPlayground -coverage -fprofile-arcs -ftest-coverage root@4b4531b84c8f:~/data/nrCascadeSim# tree bin # I install tree for example purposes bin ├── lib │   ├── cascadeProd.o │   ├── edepmath.o │   ├── isotope_info.o │   ├── libncap.so │   ├── lindhard.o │   ├── rootUtil.o │   └── weisskopf.o ├── realizeCascades ├── realizeCascades.cpp ├── regexPlayground └── regexPlayground.cpp 1 directory, 11 files root@4b4531b84c8f:~/data/nrCascadeSim# command -v realizeCascades root@4b4531b84c8f:~/data/nrCascadeSim# make install cp /home/data/nrCascadeSim/bin/realizeCascades /usr/local/bin/ cp /home/data/nrCascadeSim/bin/regexPlayground /usr/local/bin/ root@4b4531b84c8f:~/data/nrCascadeSim# command -v realizeCascades /usr/local/bin/realizeCascades ```

The install Make target hardcodes the install target directory to /usr/local/bin. Install locations can have defaults, but should allow for user input — there are many situations on remotes (example: CERN's LXPLUS) where users do not have access to /usr/local/ and the users would need to manually edit the Makefile.

Also, the clean Make target should for the same reasons not try to rm -f anything under /usr/local. This is also probably never what a user wants to have happen when make clean is run — clean targets are not uninstallers. Similarly, advocating directly for the use of sudo for installation of local software should be avoided. The .gcno generated during the build should also be cleaned in the clean target (also check for .gcda files to be safe).

A better alternative default is to assume the user is NOT root, and to attempt to install in the user's ~/bin where they will be assured write access.

Yes. The software produces reasonable results as far as I can tell and is able to pass the test suite (which includes the claims for support of Neon, Argon, Silicon, and Germanium cascades).

No performance claims are made.

Documentation

Yes. The docs include the statement

The purpose of this code is to simulate energy deposits due to cascading of energy levels following neutron capture. This code was written for use in nuclear recoil calibration for dark matter detectors, but may be useful in other particle physics applications as well, including coherent elastic neutrino nucleus scattering (CEνNS).

The docs on ReadTheDocs are not versioned beyond latest and stable. It would be great to see releases have versioned docs on ReadTheDocs too.

Yes. I have tested the examples and found that they work. They also pertain to dark matter detection physics (the area of research expertise of the authors).

The main example from the top level of the repository: ```console # time realizeCascades --verbose -n 100000 -o output.root levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt Seed used: 498136696 MT Result: 0.424727 real 0m1.495s user 0m1.345s sys 0m0.098s # root -l output.root root [0] Attaching file output.root as _file0... (TFile *) 0x55d12fd98a40 root [1] _file0->ls() TFile** output.root TFile* output.root KEY: TTree cascade;1 cascade root [2] cascade->Print() ****************************************************************************** *Tree :cascade : cascade * *Entries : 99968 : Total = 19053073 bytes File Size = 7391802 * * : : Tree compression factor = 2.58 * ****************************************************************************** *Br 0 :n : n/L * *Entries : 99968 : Total Size= 802407 bytes File Size = 7376 * *Baskets : 26 : Basket Size= 32000 bytes Compression= 108.68 * *............................................................................* *Br 1 :cid : cid/L * *Entries : 99968 : Total Size= 802467 bytes File Size = 6890 * *Baskets : 26 : Basket Size= 32000 bytes Compression= 116.35 * *............................................................................* *Br 2 :Elev : Elev[n]/D * *Entries : 99968 : Total Size= 2181158 bytes File Size = 142555 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 15.29 * *............................................................................* *Br 3 :taus : taus[n]/D * *Entries : 99968 : Total Size= 2181158 bytes File Size = 142237 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 15.32 * *............................................................................* *Br 4 :E : E[n]/D * *Entries : 99968 : Total Size= 2180903 bytes File Size = 1141056 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 1.91 * *............................................................................* *Br 5 :delE : delE[n]/D * *Entries : 99968 : Total Size= 2181158 bytes File Size = 1751576 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 1.24 * *............................................................................* *Br 6 :I : I[n]/D * *Entries : 99968 : Total Size= 2180903 bytes File Size = 421026 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 5.18 * *............................................................................* *Br 7 :Ei : Ei[n]/D * *Entries : 99968 : Total Size= 2180988 bytes File Size = 1749733 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 1.25 * *............................................................................* *Br 8 :time : time[n]/D * *Entries : 99968 : Total Size= 2181158 bytes File Size = 1880987 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 1.16 * *............................................................................* *Br 9 :Eg : Eg[n]/D * *Entries : 99968 : Total Size= 2180988 bytes File Size = 142468 * *Baskets : 81 : Basket Size= 32000 bytes Compression= 15.29 * *............................................................................* ```

The individual examples that are in the tests/bin directory build and run, and from the documentation seem to be doing what they each describe.

The examples in example-usecase/Yields_and_Resolutions.ipynb are nice and it would be great to see these get added into the docs using some of the Jupyter rendering Sphinx extensions.

Yes, the docs exist and while there are some Sphinx errors in terms of syntax, these are easy to fix.

There is an outline of the CLI API for the realizeCascades executable that is hardcoded and the nrCascadeSim library docs are generated with Doxygen.

The realizeCascades text input file format and ROOT binary output file format are also documented (I might suggest promoting I/O to its own docs section).

The use of Doxygen for the library is nice (kudos so the authors for taking this pre-review comment under suggestion) and the only advice I have about hardcoding of CLI API is to make sure that all examples in the docs for that section are rigorously tested in CI.

Yes. The author have a CI system using Travis CI that tests the software against ROOT v6.20.00 and v6.24.06 across c++11, c++14, and c++17 on various Ubuntu Linux distributions and a subset of those configurations on various MacOS releases.

The code coverage at the time of the review is 82%. While low, this is acceptable for the review (though I'd encourage the authors to keep up their nice work of improving the coverage).

As already mentioned in https://github.com/villano-lab/nrCascadeSim/issues/75, the code is lacking regressions tests. It would be good if there could be at least one in the test suite and if the authors could plan to add more in the future.

While changing CI providers is not something that is relevant for this review, it should be noted that Travis CI now (2020 onwards) offers a open source projects a one time non-renewable 10,000 build credit. While it does not impact this review, the authors will need to consider how they plan to continue testing their software once this on time limit has been reached.

Yes. There is a CONTRIBUTING.md and CODE_OF_CONDUCT.md and the README mentions

For questions, support, bug reports, or other suggestions, please open an issue.

Additionally, there is a Contact & Support section of the docs and

# realizeCascades --help | tail -n 3
Report bugs to: villaa-at-gmail-dot-com
realizeCascades (nrCascadeSim) home page: <https://github.com/villano-lab/nrCascadeSim>
General help nrCascadeSim Software: <https://nrcascadesim.readthedocs.io/en/latest/>

Software paper

Yes. The introduction does a good job of this.

Yes.

Yes. They compare nrCascadeSim to Geant4 and FIFRELIN in a fair manner.

The authors don't mention this, but it is worth noting that Geant4 is a considerably larger project and given the large number of use cases it is meant to handle can be much more cumbersome to use. While Geant4 is fully open source, it is not clear from searches if there are open source implementations of the FIFRELIN Monte Carlo simulator. If not, nrCascadeSim provides an advantage by being open source.

The paper is of good quality and of the proper scope following JOSS guidelines.

Yes.

Suggested revisions before JOSS publication

@lucydot, I think the paper and the project are in overall good shape. My recommendation is that once the following revisions are addressed (I'll open up GitHub Issues for them) the project be accepted for publication pending your approval. Though we also still need @altavir's written review as well.

Code

Paper

Suggested revisions outside of review

These suggestions should not be considered inside the scope of the review and editors should not view their completion as a dependency for approval.

lucydot commented 2 years ago

Thank-you @matthewfeickert for your extremely thorough review!

@villaa it looks like it is over to you: the message above makes clear what actions are needed, https://github.com/villano-lab/nrCascadeSim/issues/75 is also a sticking point for @altavir. I can see that you and your team are currently working on this @villaa.

@altavir could I ask you to update your tick boxes at the top of the page, if you are in a position to do so? I can then better assess how close we are to completing the review process.

matthewfeickert commented 2 years ago

(I just went to open up Issues and I see that @gerudo7 was speedy and beat me to it. :slightly_smiling_face: Thanks! I'll add some comments on them to try to make them more helpful.)

altavir commented 2 years ago

@lucydot I've marked all but the State of the field box. I think that a comparison of the results is required for it.

lucydot commented 2 years ago

Hello @altavir - regarding state of the field: we do not usually require that there is a quantative comparison made with other software. This section can include a qualitative comparison of functionality against other existing software (as is the case here), however if performance claims are made (e.g. that it runs faster) this must be backed up. As performance claims are not made for this code (from what I can see - please let me know if I am wrong), my view is that a comparison of results to other software is not needed.

However, as mentioned by Matthew above, and yourself in https://github.com/villano-lab/nrCascadeSim/issues/75, regression tests are needed to compare results from different versions of this software.

altavir commented 2 years ago

@lucydot the problem in this particular case is the only aim of this piece of software is a simulation of a specific process. Without a comparison, it is not possible to decide if the aim of the development is achieved. In simulations it is quite easy to get completely wrong result even with correct theory. Therefore, I believe that at least some comparison should be made. Eithere with an experiment data (it is better) or with other software.

If the results are consistent with GEANT, but it worls much faster, it is OK. But if the results are significantly different, additional research is required to understand, which one is wrong (and GEANT could and is wrong on many occasions).

villaa commented 2 years ago

@lucydot @altavir we are committed to putting in the comparison with Geant4 at this point in any case. We also already have regression tests in the queue to be released. So, perhaps those things will be enough to satisfy all.

lucydot commented 2 years ago

@villaa I can see you are currently working on the Geant4 comparison testing (villano-lab/nrCascadeSim#75) - do you have a rough timescale for completion? Your software has travelled so far on the JOSS journey, I want to keep the train moving! 🚂

villaa commented 2 years ago

@villaa I can see you are currently working on the Geant4 comparison testing (villano-lab/nrCascadeSim#75) - do you have a rough timescale for completion? Your software has travelled so far on the JOSS journey, I want to keep the train moving! 🚂

tl;dr : probably a couple of days to a week.

@lucydot yes, thanks. for noticing. @gerudo7 is working on finalizing the Geant4 comparison and working off of PR # 98. The comparison plots have been finalized and from what I understand the work remaining is to get a reasonable exposition of these facts into the documentation and the paper, is this correct @gerudo7 ? I would expect this to take just a few days to finish, do you agree @gerudo7 ?

For my part I have updated the build system to CMake and merged that to develop. In so doing I think that took care of the other important issues--namely cleaning the code coverage files and allowing a "local" install.

All those features will be released together--probably with a bump to the middle number in our versioning scheme because of the overhaul of the build system.

nuclearGoblin commented 2 years ago

@gerudo7 is working on finalizing the Geant4 comparison and The comparison plots have been finalized and from what I understand the work remaining is to get a reasonable exposition of these facts into the documentation and the paper, is this correct @gerudo7 ? I would expect this to take just a few days to finish, do you agree @gerudo7 ?

@villaa Yes. The Geant4 comparison feature branch is just about ready to be merged back, so I think a few days is a reasonable timescale to incorporate that information into the paper.

villaa commented 2 years ago

Ok @lucydot @altavir and @matthewfeickert we have just released v1.4.0 -- the build system has been migrated to CMake and we have clearly documented comparisons to Geant4. These were the issues that were considered necessary for us to move on.

Huge thanks to @gerudo7 who took care of the Geant4 comparison and generally did a lot of work to get our code up to par.

altavir commented 2 years ago

Looks good now. The comparison with GEANT4 shows a lot of differences, but it is for users to decide if the approximation is right or wrong in their case. The change of build configuration is also nice.

matthewfeickert commented 2 years ago

I'll try to get this done before Wednesday (2022-02-16) night as I had only minor suggestions but I will make sure this is reviewed before EOD on Friday (2022-02-18).

matthewfeickert commented 2 years ago

@lucydot Can you please have whedon set v1.4.0 as version, as that's the final one we're reviewing now?

matthewfeickert commented 2 years ago

@whedon generate pdf from branch v1.4.0

whedon commented 2 years ago 
Attempting PDF compilation from custom branch v1.4.0. Reticulating splines etc...
whedon commented 2 years ago

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