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

whedon commented 2 years ago

Submitting author: @villaa (Anthony Villano) Repository: https://github.com/villano-lab/nrCascadeSim Version: v1.2.2 Editor: @lucydot Reviewers: @matthewfeickert, @altavir Managing EiC: Arfon Smith

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

Author instructions

Thanks for submitting your paper to JOSS @villaa. Currently, there isn't an JOSS editor assigned to your paper.

@villaa if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @whedon is here to help you find and assign reviewers and start the main review. To find out what @whedon can do for you type:

@whedon commands

whedon commented 2 years ago

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

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

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.14 s (761.5 files/s, 210369.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            529
Python                           8            164            269            528
Markdown                        14            188              0            512
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           4667           5238          19102
-------------------------------------------------------------------------------

Statistical information for the repository '856665b8b8b909e5606875f3' was
gathered on 2021/11/19.
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:

arfon commented 2 years ago

@villaa – I see this is a resubmission based on the feedback from an earlier review. Before we assign an editor here, could I ask you to provide a detailed summary of the changes that you've made since the last review (this should help expedite the process on our part).

villaa commented 2 years ago

@afron sure, the list of things that were done can be found here. All the work from v1.1.0. to v1.2.2 has been done in response to the earlier review.

Kevin-Mattheus-Moerman commented 2 years ago

@arfon could you judge this, based on what you looked at already, or do you want me to take a look?

arfon commented 2 years ago

@arfon could you judge this, based on what you looked at already, or do you want me to take a look?

If you could that would be great. Thanks @Kevin-Mattheus-Moerman. Ideally @mbobra would be able to do this but I think she's on leave until January.

Kevin-Mattheus-Moerman commented 2 years ago

@arfon okay thanks. I cannot thoroughly check if all functionality and documentation issues are fully resolved wrt the previous review (since this software is outside of my area of expertise) but it seems we may be ready to move on based on @villaa 's comments above and the release notes, which seem to tick off most of issues mentioned in the previous review.

@villaa it would be helpful if rather than the less human readable release notes to summarize the required changes from the previous review and to link to where they were resolved. Although I will proceed to find an editor as this point, I think such a list/summary would be very helpful to whoever has to pick this up to handle as editor. Thanks.

Kevin-Mattheus-Moerman commented 2 years ago

@pibion this seems to me to be exactly your cup of tea! It looks like you are handling many other submissions at the moment, would you be able to handle this submission once you are available?

Kevin-Mattheus-Moerman commented 2 years ago

@arfon @villaa I'll waitlist this submission for now as most of our physics related editors are fully occupied and it does look like @pibion would be a great fit.

Kevin-Mattheus-Moerman commented 2 years ago

@villaa while we wait for an editor you have some time to create the clearest possible description here of what was needed in the former review and how the issues raised were addressed. Does that sound good?

pibion commented 2 years ago

@Kevin-Mattheus-Moerman I'd absolutely love to edit this submission - it's exactly my cup of tea :)

There is a potential COI - I'm @villaa's spouse. I feel like I can still fairly edit, but I also don't want to overstep.

Kevin-Mattheus-Moerman commented 2 years ago

@pibion okay we do consider that a (actual or perceived) COI so I will look for an alternative editor. Thanks for getting back to me.

villaa commented 2 years ago

@villaa while we wait for an editor you have some time to create the clearest possible description here of what was needed in the former review and how the issues raised were addressed. Does that sound good?

@Kevin-Mattheus-Moerman , sure I can provide that. The previous submission had one reviewer give detailed comments and the authors believe we have addressed those comments in full. In the previous review the second reviewer had not given any comments and the author team had made the mistake of waiting on the first reviewer's comments for when the second reviewer responded. This made the review take too long from the perspective of the first reviewer and they suggested we withdraw and re-submit when their comments were addressed. See the review thread here.

The key issues that were raised are below, they have been addressed. The reviewer was @matthewfeickert so they can comment on how satisfactory these updates are if they want to.

although it is not required the reviewer suggested to use a workflow to include github issues and pull requests. We now work exclusively in this mode.
Issue9 addressed the lack of contributing protocols. This was addressed in PR19 and closed.
Issue3 addressed the lack of a statement of need. It suggested to put it in the README. The README.md in the repository is now shorter but the paper itself has a clear statement of need. Furthermore, the documentation, which has been migrated to readthedocs is clear up-front about the applicability of the program.
Issue4 the ROOT citation was. incorrect and that was fixed in PR21
Issue5 We use an external (older) version of the Mersenne Twister algorithm which needed to have its license included. We included that with PR13
Issue7 pointed out that we had an examples directory with no documentation within it and that the .ipynb file did not count as a demo but only "a nice visualization." We have since included a demo script in PR40
Issue8 was for typos in the initial README.md top-level documentation and lack of documentation for the custom input "level" files. The typos along with better documentation for the level files were included in PR20. Furthermore this documentation was migrated and further clarified when we moved to readthedocs.
Issue10 was for us to construct an automated testing system. We did this through TravisCI in PR17. We removed official support for OSX (there are still some lingering issues with that) and only formally support Linux Ubuntu Xenial distro. This is the distro that is used in our TravisCI.
Issue11 requested that the paper be made sorter and for us to remove a figure because it was more documentation. This was fixed in PR25
Issue12 said that our main executable realizeCascades was overly verbose. We included verbosity options in PR16.
Issue14 requested a requirements.txt in the example directory, this was added in PR15.
Issue23 (not suggested by reviewer) was brought up by us as a way to clean up and expand the documentation by using readthedocs. This was resolved in PR38.
Issue24 (not suggested by reviewer) was to get a Zenodo DOI for the code. This was done in PR30.

Kevin-Mattheus-Moerman commented 2 years ago

@whedon invite @lucydot as editor

whedon commented 2 years ago

@lucydot has been invited to edit this submission.

villaa commented 2 years ago

@lucydot and @Kevin-Mattheus-Moerman , Is there any way I can help get this into review? Or is there something I can do to otherwise move things along. Thanks!

matthewfeickert commented 2 years ago

The key issues that were raised are below, they have been addressed. The reviewer was @matthewfeickert so they can comment on how satisfactory these updates are if they want to.

Hey sorry just saw the email that got sent today (I missed the first one in my slew of GitHub notifications it seems). I'll take a look at this over the weekend and make any comments here if that works for everyone.

Kevin-Mattheus-Moerman commented 2 years ago

@lucydot are you able to handle this submission?

Kevin-Mattheus-Moerman commented 2 years ago

@whedon invite @lucydot as editor

whedon commented 2 years ago

@lucydot has been invited to edit this submission.

lucydot commented 2 years ago

Hello @Kevin-Mattheus-Moerman @villaa apologies I managed to miss the first invite. I can edit this submission.

lucydot commented 2 years ago

@whedon assign @lucydot as editor

whedon commented 2 years ago

OK, the editor is @lucydot

lucydot commented 2 years ago

Hi @matthewfeickert I've just taken a look at the previous review thread and can see you have invested some time into reviewing this piece of work - if you are available to give your thoughts on the changes made for this re-submission that would be very useful.

villaa commented 2 years ago

@matthewfeickert -- has there been any progress on this? I'm eager to get this paper into review.

villaa commented 2 years ago

@lucydot I have consulted the reviewer spreadsheet and I think perhaps the following is a reasonable pool of reviewers to pull from: @temken, @zonca, @bradkav, @sskutnik, @williamfgc, @andrewryh

zonca commented 2 years ago

Bad timing, can't make it

bradkav commented 2 years ago

Sorry - I don't think I can take anything else on before Christmas.

matthewfeickert commented 2 years ago

Hi @matthewfeickert I've just taken a look at the previous review thread and can see you have invested some time into reviewing this piece of work - if you are available to give your thoughts on the changes made for this re-submission that would be very useful.

:wave: Hi @lucydot. Sure, I ran through a mini-review tonight following my previous set of review comments. Below are my notes on nrCascadeSim v1.2.3.

The TL;DR though is that the team has made lots of great improvements here since their last submission and I think that all of my comments that I would raise in the review could be addressed very quickly, so I would recommend that this submission move forward to review now.

I've reviewed this software 3 times now (including these notes), so I'm happy to serve as a reviewer again as it would probably speed up the process as I have very targeted suggestions at this point. I'll let you and the team decide if that sounds good.

Notes

As always, these notes are following the JOSS paper review criteria. For brevity, I'm making comments only on major points that were brought up in the previous rounds of review.

General Checks

License issues are resolved RE: Msersenne Twister prime algorithm. :+1:

Functionality

[ ] Installation: Does installation proceed as outlined in the documentation?
- No. The Makefile has hardcoded assumptions on the C++ standard that will be used, while they should instead rely on the information provided by root-config --cflags. Here the Makefile has hardcoded c++14. If this software wants to make the decision to enforce an uppper bound on the C++ standards they support that's not fundamentally wrong, but it needs to be made explicitly clear to the user in advance.

Reproducible example of failing with ROOT compiled against c++17:

$ docker run --rm -ti -v $PWD:$PWD -w $PWD atlasamglab/stats-base:root6.24.06
# git clone --depth 1 https://github.com/villano-lab/nrCascadeSim.git --branch v1.2.3 --single-branch
# cd nrCascadeSim/
# 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 Oct 19 2021, 19:44:00
From tags/v6-24-06@v6-24-06
# root-config --cflags
-pthread -std=c++17 -m64 -I/usr/local/venv/include
# make
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//isotope_info.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv isotope_info.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//weisskopf.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv weisskopf.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//lindhard.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv lindhard.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//cascadeProd.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
src//cascadeProd.c: In function ‘cri* Cascade(int, int, double, int, double*, double*, double, MTRand*)’:
src//cascadeProd.c:1113:1: warning: control reaches end of non-void function [-Wreturn-type]
 1113 | }
      | ^
mv cascadeProd.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//edepmath.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv edepmath.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//rootUtil.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
In file included from /usr/local/venv/include/TString.h:29,
                 from /usr/local/venv/include/TNamed.h:26,
                 from /usr/local/venv/include/TDictionary.h:44,
                 from /usr/local/venv/include/TClass.h:23,
                 from /usr/local/venv/include/TTree.h:36,
                 from inc/rootUtil.h:3,
                 from src//rootUtil.c:8:
/usr/local/venv/include/ROOT/RStringView.hxx:84:17: error: expected type-specifier
   84 |        operator std::string_view() const { return std::string_view(fData,fLength); }
      |                 ^~~
In file included from /usr/local/venv/include/TNamed.h:26,
                 from /usr/local/venv/include/TDictionary.h:44,
                 from /usr/local/venv/include/TClass.h:23,
                 from /usr/local/venv/include/TTree.h:36,
                 from inc/rootUtil.h:3,
                 from src//rootUtil.c:8:
/usr/local/venv/include/TString.h:115:13: error: expected type-specifier
  115 |    operator std::string_view() const { return std::string_view(Data(),fExtent); }
      |             ^~~
/usr/local/venv/include/TString.h:280:32: error: ‘string_view’ in namespace ‘std’ does not name a type
  280 |    explicit TString(const std::string_view &sub);
      |                                ^~~~~~~~~~~
/usr/local/venv/include/TString.h:280:27: note: ‘std::string_view’ is only available from C++17 onwards
  280 |    explicit TString(const std::string_view &sub);
      |                           ^~~
/usr/local/venv/include/TString.h:317:37: error: ‘string_view’ in namespace ‘std’ does not name a type
  317 |    TString    &operator=(const std::string_view &s);
      |                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:317:32: note: ‘std::string_view’ is only available from C++17 onwards
  317 |    TString    &operator=(const std::string_view &s);
      |                                ^~~
/usr/local/venv/include/TString.h:444:9: error: ‘string_view’ in namespace ‘std’ does not name a type
  444 |    std::string_view View() const { return std::string_view(GetPointer(),Length()); }
      |         ^~~~~~~~~~~
/usr/local/venv/include/TString.h:444:4: note: ‘std::string_view’ is only available from C++17 onwards
  444 |    std::string_view View() const { return std::string_view(GetPointer(),Length()); }
      |    ^~~
In file included from /usr/local/venv/include/TNamed.h:26,
                 from /usr/local/venv/include/TDictionary.h:44,
                 from /usr/local/venv/include/TClass.h:23,
                 from /usr/local/venv/include/TTree.h:36,
                 from inc/rootUtil.h:3,
                 from src//rootUtil.c:8:
/usr/local/venv/include/TString.h:839:53: error: ‘string_view’ in namespace ‘std’ does not name a type
  839 | inline Bool_t operator==(const char *s1, const std::string_view &s2)
      |                                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:839:48: note: ‘std::string_view’ is only available from C++17 onwards
  839 | inline Bool_t operator==(const char *s1, const std::string_view &s2)
      |                                                ^~~
/usr/local/venv/include/TString.h:839:15: error: ‘Bool_t operator==(const char*, const int&)’ must have an argument of class or enumerated type
  839 | inline Bool_t operator==(const char *s1, const std::string_view &s2)
      |               ^~~~~~~~
/usr/local/venv/include/TString.h:844:37: error: ‘string_view’ in namespace ‘std’ does not name a type
  844 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:844:32: note: ‘std::string_view’ is only available from C++17 onwards
  844 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |                                ^~~
/usr/local/venv/include/TString.h:844:15: error: ‘Bool_t operator==(const int&, const char*)’ must have an argument of class or enumerated type
  844 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |               ^~~~~~~~
/usr/local/venv/include/TString.h:857:37: error: ‘string_view’ in namespace ‘std’ does not name a type
  857 |   std::string printValue(const std::string_view* val);
      |                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:857:32: note: ‘std::string_view’ is only available from C++17 onwards
  857 |   std::string printValue(const std::string_view* val);
      |                                ^~~
In file included from inc/rootUtil.h:4,
                 from src//rootUtil.c:8:
/usr/local/venv/include/TFile.h:327:45: error: ‘std::string_view’ has not been declared
  327 |    static Bool_t       SetCacheFileDir(std::string_view cacheDir, Bool_t operateDisconnected = kTRUE,
      |                                             ^~~~~~~~~~~
/usr/local/venv/include/TFile.h: In static member function ‘static Bool_t TFile::SetCacheFileDir(ROOT::Internal::TStringView, Bool_t, Bool_t)’:
/usr/local/venv/include/TFile.h:326:36: error: ‘string_view’ is not a member of ‘std’
  326 |      { return SetCacheFileDir(std::string_view(cacheDir), operateDisconnected, forceCacheread); }
      |                                    ^~~~~~~~~~~
/usr/local/venv/include/TFile.h:326:36: note: ‘std::string_view’ is only available from C++17 onwards
make: *** [Makefile:39: /tmp/nrCascadeSim/bin/lib/rootUtil.o] Error 1

If the hardcoded C++ flags are changed to reflect the version of C++ used to compile ROOT then things will pass (this is why the project CI passes, as only c++14 is tested).

...
# sed -i 's/c++14/c++17/g' Makefile
# make clean && make  # Skipping lots of build warning here for readability
# tree bin
bin
├── lib
│   ├── cascadeProd.o
│   ├── edepmath.o
│   ├── isotope_info.o
│   ├── libncap.so
│   ├── lindhard.o
│   ├── rootUtil.o
│   └── weisskopf.o
├── realizeCascades
└── realizeCascades.cpp

1 directory, 9 files
# command -v realizeCascades  # Just showing that nothing is in PATH yet as expected
# make install
cp /tmp/nrCascadeSim/bin/realizeCascades /usr/local/bin/
# command -v realizeCascades
/usr/local/bin/realizeCascades

This should not be the responsibility of the user, but should be discoverable and covered in tests.

The install Make target also hardcodes the install target directory to /usr/local/bin. Install locations can have defaults, but should allow for user input. Similarly, advocating directly for the use of sudo for installation of local software should be avoided.

Documentation

The docs have improved significantly, so great work there! There are some Sphinx errors in terms of syntax, but I don't think that anyone has perfectly typeset anything in Sphinx in the first go, so that shouldn't be held against anyone. These are easy to fix on an afternoon with a coffee/tea too.

There is an outline of the API now too (another great improvement). It is hardcoded in the docs .rst files though, which is not great as this is extremely brittle and prone to drift. What would be quite a bit nicer would be to generate the docs from the source code itself (for C++ code the obvious method is Doxygen and there are tutorials and examples out there on how to build Doxygen annotated projects in Sphinx.)

The API documentation section of the review guidelines mentions

the software API is documented to a suitable level.

So while as a library maintainer I can strongly recommend that this be done, it isn't a requirement for the review — but it would certainly improve the usefulness of the API docs.

Example usage

The first example that a user encounters in the Executables of nrCascadeSim section of the docs fails. It will run without producing warning or errors, but if run from the top level of the project repo so that levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt is in the given example path the content of the resulting ROOT TTree in the file is empty

# 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: 339870340
MTRand: 0x55c868a47d70
# ls -lh output.root
-rw-r--r-- 1 root root 4.6K Dec 10 07:08 output.root
# root -l output.root
root [0]
Attaching file output.root as _file0...
(TFile *) 0x55c3f3ee8dc0
root [1] _file0->ls()
TFile**     output.root
 TFile*     output.root
  KEY: TTree    cascade;1   cascade
root [2] cascade->Print()
******************************************************************************
*Tree    :cascade   : cascade                                                *
*Entries :        0 : Total =             329 bytes  File  Size =        188 *
*        :          : Tree compression factor =   1.00                       *
******************************************************************************
root [3]

This doesn't seem correct given that the example says it will:

simulate 100000 events for 28Si and output them to a file...

The examples section in the docs are a nice improvement! They center around an

level input file called inputfile.txt of the correct format

As the project includes over 30 levelfiles it would be nice to show specific run examples and output in the docs as well rather than just example source.

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
- There's CI on Travis-CI against one version of ROOT now. That's a big improvement. It would be nice if there was a known lower bound version of ROOT tested in terms of API dependence and a latest version of ROOT tested as well. Also at the moment the tests are testing the examples, but there isn't coverage reported for the codebase. This isn't a requirement, but it is nice to have so users can have some first heuristic of confidence in the strength of the tests / API stability.
[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
- There is a CONTRIBUTING.md and CODE_OF_CONDUCT.md now! :tada:

Software paper

I didn't review the paper tonight. That can be done in the review.

temken commented 2 years ago

@lucydot I have consulted the reviewer spreadsheet and I think perhaps the following is a reasonable pool of reviewers to pull from: @temken, @zonca, @bradkav, @sskutnik, @williamfgc, @andrewryh

Thanks for the consideration. Unfortunately, I work only part-time until early next year, and I cannot accept more referee requests until then. Apologies!

williamfgc commented 2 years ago

I'll be happy to take a look in January, I'm taking a long break around the holidays with little computer access. Thanks also for the consideration.

villaa commented 2 years ago

@lucydot I have added the following to the pool of potential reviewers that I've cultivated: @danehkar, @smsharma, @AstroBarker, @ziotom78, @manodeep, @kostunin, @aureliocarnero .

Would any of those people be available for this review?

kostunin commented 2 years ago

This is bit out of my expertise (will take longer to get into problem then usual), but I can recommend @altavir as a reviewer

smsharma commented 2 years ago

@lucydot I have added the following to the pool of potential reviewers that I've cultivated: @danehkar, @smsharma, @AstroBarker, @ziotom78, @manodeep, @kostunin, @aureliocarnero .

Would any of those people be available for this review?

Similarly somewhat out of my scope, thank you for considering!

ziotom78 commented 2 years ago

@lucydot I have added the following to the pool of potential reviewers that I've cultivated: @danehkar, @smsharma, @AstroBarker, @ziotom78, @manodeep, @kostunin, @aureliocarnero .

Would any of those people be available for this review?

It's outside my domain as well, and I am currently involved in two other reviews for JOSS, so I have to skip this one, sorry…

altavir commented 2 years ago

Hi, thanks for a vote of confidence, @kostunin. I am not a fan of the C language and I am not familiar with the review process here, but I can review code both in terms of particle physics and software design.

matthewfeickert commented 2 years ago

I am not a fan of the C language and I am not familiar with the review process here, but I can review code both in terms of particle physics and software design.

@altavir, forgive me if I'm wrong, but it doesn't look like you are a registered JOSS reviewer. If you are interested in reviewing for JOSS (that's great!) please first read the Reviewing for JOSS overview and then register to become a JOSS reviewer. (Also hi from a fellow particle physicist! :wave: :slightly_smiling_face:)

edit: I've just been informed by a member of the JOSS Editorial Board that reviewers do not need to be registered to review for JOSS. Though it would be great to have you signed up!

altavir commented 2 years ago

@matthewfeickert OK, I never knew about the opportunity before Dmitriy tagged me. I think it is an interesting idea in general - I mean the community to make better scientific software. Though I think it requires more serious measures (code quality expertise, not only documentation quality). I've sent my registration info.

villaa commented 2 years ago

@matthewfeickert I have made issues (5) out of your comments above so please don't open duplicates. They are labeled with "JOSS pre-review." The team will get to those things. Please let me know if there is a timescale beyond which you will not be willing to check them for correctness.

villaa commented 2 years ago

@lucydot it seems like there are some options for reviewers, or I could try to help find others. Please advise; I would like to get this paper into review.

manodeep commented 2 years ago

@lucydot I have added the following to the pool of potential reviewers that I've cultivated: @danehkar, @smsharma, @AstroBarker, @ziotom78, @manodeep, @kostunin, @aureliocarnero .

Would any of those people be available for this review?

Outside of my domain area of expertise - I will have to decline.

lucydot commented 2 years ago

Hi @villaa - ok it seems we can assign two reviewers to this: @altavir who can give a viewpoint on particle physics and software design, and @matthewfeickert who seems to have the C side of things under control 😉

I'll ask our bot Whedon to start a review issue thread (this is the pre-review thread) - the review guidelines will be listed at the top of that new thread.

@altavir welcome to JOSS! Please ping me in the new review thread with any questions you have about the process. There are review guidelines and a checklist at the new thread (the link will appear below), and there is further reviewer documentation here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

We're asking for reviews to be complete within a 4-6 week period however I think given that many of us will be taking a break over Christmas a 1-2 week delay might be expected and is reasonable. As the review is usually an iterative process (rather than something you do in one sitting) it's best for reviews to start earlier in that timeline.

lucydot commented 2 years ago

@whedon assign @matthewfeickert as reviewer

whedon commented 2 years ago

OK, @matthewfeickert is now a reviewer

lucydot commented 2 years ago

@whedon add @altavir as reviewer

whedon commented 2 years ago

OK, @altavir is now a reviewer

lucydot commented 2 years ago

@whedon start review

openjournals / joss-reviews