search

openjournals / joss-reviews

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

[REVIEW]: PropPy -- Correlated random walk propagation of cosmic rays in magnetic turbulence #4243

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@reichherzerp<!--end-author-handle-- (Patrick Reichherzer) Repository: https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy Branch with paper.md (empty if default branch): joss_paper Version: v1.1.1 Editor: !--editor-->@christinahedges<!--end-editor-- Reviewers: @carmeloevoli, @amitseta90 Archive: 10.5281/zenodo.6573056

Status

status

Status badge code:

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

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

@carmeloevoli & @amitseta90, 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 @christinahedges 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 @amitseta90

📝 Checklist for @carmeloevoli

editorialbot commented 2 years ago

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 commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.15 s (298.1 files/s, 75192.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          32            924           1164           2954
Jupyter Notebook                 9              0           4891            603
TeX                              1             34              0            424
Markdown                         2            101              0            247
Bourne Shell                     1              0              0              9
-------------------------------------------------------------------------------
SUM:                            45           1059           6055           4237
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/0004-637X/806/2/217 is OK
- 10.1088/1674-4527/16/10/162 is OK
- 10.3847/1538-4357/ab643b is OK
- 10.22323/1.395.0468 is OK
- 10.1098/rsif.2008.0014 is OK
- 10.1086/148912 is OK
- 10.1063/1.4928940 is OK
- 10.1063/1.4807033 is OK
- 10.1088/1475-7516/2017/06/046 is OK
- 10.1088/1475-7516/2016/05/038 is OK
- 10.5281/zenodo.5959220 is OK
- 10.5281/zenodo.5959618 is OK
- 10.22323/1.395.0978 is OK
- 10.1093/mnras/staa1650 is OK
- 10.3847/1538-4357/aa603a is OK
- 10.1088/1475-7516/2017/02/015 is OK
- 10.1086/306470 is OK
- 10.1093/mnras/staa2533 is OK
- 10.1103/PhysRevD.65.023002 is OK
- 10.1086/307452 is OK
- 10.1007/s42452-021-04891-z is OK
- 10.1016/j.physrep.2020.05.002 is OK
- 10.3847/1538-4365/ac1517 is OK
- 10.3847/2041-8213/aa6aa6 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago

Wordcount for paper.md is 1175

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

christinahedges commented 2 years ago

@reichherzerp, @carmeloevoli, @amitseta90 – This is the review thread for the PropPy paper. Please don't hesitate to message me here if you have questions (use @christinahedges). ✉️

Please read the "Reviewer instructions & questions" in the first comment above to get started. If you get lost, you can also see the reviewer guidelines. You will have to generate your review "checklist" by adding the comment @editorialbot generate my checklist to this thread.

To review for JOSS, @carmeloevoli and @amitseta90 will step through that checklist for PropPy and assess the package. As you go over the submission, please check any items that you feel have been satisfied. If you are concerned about a requirement, please discuss it here on this thread 🧵 . Feel free to post about questions/concerns as they come up as you go through your review. Discussion between the authors/reviewers and myself is encouraged!

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention this issue (https://github.com/openjournals/joss-reviews/issues/4243) so that a link is created to this thread (and I can keep an eye on what is happening).

We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule. When you're finished with your checklist, leave a comment and @ me to let everyone know your review is complete.

amitseta90 commented 2 years ago

Review checklist for @amitseta90

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

christinahedges commented 2 years ago

Hi @carmeloevoli and @amitseta90, I am gently checking in to see how your review is going. @carmeloevoli I see you haven't started your checklist yet, please let me know if you're having any trouble with it.

amitseta90 commented 2 years ago

Dear Christina,

I will working on it. It should be done by the end of the next week.

Thanks, Best regards, Amit

Dr. Amit Seta (he/him) @. ANU id: @. https://www.mso.anu.edu.au/~amitseta

On 2 Apr 2022, at 3:27 am, Christina Hedges @.***> wrote:

Hi @carmeloevoli https://github.com/carmeloevoli and @amitseta90 https://github.com/amitseta90, I am gently checking in to see how your review is going. @carmeloevoli https://github.com/carmeloevoli I see you haven't started your checklist yet, please let me know if you're having any trouble with it.

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4243#issuecomment-1086113792, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHJNSGWHHLX2RM7TK3IBUJLVC4PW7ANCNFSM5QZOLKOA. You are receiving this because you were mentioned.

carmeloevoli commented 2 years ago

Review checklist for @carmeloevoli

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

carmeloevoli commented 2 years ago

@reichherzerp: The link to the plot "running diffusion coefficient" in the README page is broken: https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy

It would be useful to have this plot to finish my report.

reichherzerp commented 2 years ago

@carmeloevoli Thank you, these are links to figures that I have now corrected. Previously they had pointed to my private space, which is why only I could see them.

christinahedges commented 2 years ago

It looks like @carmeloevoli has finished their review of this work, and has completed their checklist. @carmeloevoli, can you sound off in the comments that you are happy to finish your review of this submission?

@amitseta90, you seem to have completed half the review of this work. Can you chime in on how you are doing with your review?

amitseta90 commented 2 years ago

Dear @christinahedges,

I had already completed the coding part of the review and also read through the paper now. So, all done and looks very good. Happy to sign off the paper.

carmeloevoli commented 2 years ago

It looks like @carmeloevoli has finished their review of this work, and has completed their checklist. @carmeloevoli, can you sound off in the comments that you are happy to finish your review of this submission?

Yes, very nice work! I anticipate a lot of interest for this code in our community.

reichherzerp commented 2 years ago

Dear @amitseta90 and @carmeloevoli, I appreciate your effort of reviewing the software package and the paper. Thanks a lot!

christinahedges commented 2 years ago

Thank you @amitseta90 and @carmeloevoli for your review!

@reichherzerp it seems like both reviewers have checked off all their items. I had a quick look at your package too and I wanted to provide the following feedback to you

  1. You have the JOSS paper for your tool in your master branch. The paper is quite big (100s of Mb) and this will give you two problems, a. it becomes cumbersome to version control and b. it means that when i want to install your tool I will always end up grabbing a large file which is the copy of your paper. You might consider either making it smaller, or putting it on a separate branch before we accept the submission. I believe your ipython notebooks are also making this package large (400Mb!) so you might want to think about that too, it's helpful to make sure packages are small for users.
  2. You've written very thorough docstrings for most of the code which is great. You might want to think about adding a readthedocs or other website which will show these API docs, so people can easily search and learn about these functions.
  3. I tried to install your tool to run the tests, but I am actually having some problems with getting it to install. That may be my machine, and I can keep trying on that front. So far I haven't been able to run your tests, but you might consider continuous integration of your tests to demonstrate to users that the tests pass.

These are all suggestions, and none of these are strictly necessary to pass the review. But I wanted to raise them as you may be interested in addressing these before we finish the review, and I'm happy to give you pointers on how to address each of them.

If you decide you would like to proceed with submission, please make a tagged release and archive, and report the version number and archive DOI here. Please make sure on the archive you have the correct meta data (author names in particular!) I have checked through your paper submission and it seems to have all the key elements, I will also check if there are any small wording issues there.

christinahedges commented 2 years ago

@editorialbot generate pdf

christinahedges commented 2 years ago

@editorialbot check references

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

OK DOIs

- 10.1088/0004-637X/806/2/217 is OK
- 10.1088/1674-4527/16/10/162 is OK
- 10.3847/1538-4357/ab643b is OK
- 10.22323/1.395.0468 is OK
- 10.1098/rsif.2008.0014 is OK
- 10.1086/148912 is OK
- 10.1063/1.4928940 is OK
- 10.1063/1.4807033 is OK
- 10.1088/1475-7516/2017/06/046 is OK
- 10.1088/1475-7516/2016/05/038 is OK
- 10.5281/zenodo.5959220 is OK
- 10.5281/zenodo.5959618 is OK
- 10.22323/1.395.0978 is OK
- 10.1093/mnras/staa1650 is OK
- 10.3847/1538-4357/aa603a is OK
- 10.1088/1475-7516/2017/02/015 is OK
- 10.1086/306470 is OK
- 10.1093/mnras/staa2533 is OK
- 10.1103/PhysRevD.65.023002 is OK
- 10.1086/307452 is OK
- 10.1007/s42452-021-04891-z is OK
- 10.1016/j.physrep.2020.05.002 is OK
- 10.3847/1538-4365/ac1517 is OK
- 10.3847/2041-8213/aa6aa6 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot 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:

reichherzerp commented 2 years ago

Hi @christinahedges, many thanks for the detailed comments and suggestions for improvement!

  1. I have now significantly reduced the package size (factor >10). I have implemented this partly through your suggestions by putting the paper in its own branch. I have also done the same for the comparisons with CRPropa. In addition, I have purged the history of old, large simulation files according to the GitLab instructions (https://docs.gitlab.com/ee/user/project/repository/reducing_the_repo_size_using_git.html). (Some old simulation files were still present in the git history)
  2. I created the documentation in readthedocs style and hosted it on https://proppy-doc.web.app. It is linked from the GitLab readme.
  3. Whereas I think your suggestion for the CI is great, to actually implement it I would have to contact the system administrators of the GitLab instance I use and configure the runners. I have included the suggestion in an issue and would take care of it later. Instead, I have tried PropPy on various computers and servers available to me in different virtual environments and have been able to reproduce installation problems under certain conditions and package versions. By restricting the package versions in the requirement.txt file and making adjustments in the setup.py file, I was able to fix all the problems. However, if you still have problems, I would be happy if you could post the error in an issue in Gitlab and I will take care of it.

After all the changes, I went through all the tutorials again and checked that they were correct. The tests are also running (sometimes with some warnings, which are unproblematic). Given the progress, I would, if you don't mind, continue with the submission early next week.

reichherzerp commented 2 years ago

@christinahedges the software is archived at 10.5281/zenodo.6573031 under the version v1.1.1. The paper is under the joss_paper branch

christinahedges commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf. Paper file not found.

christinahedges commented 2 years ago

@editorialbot set joss_paper as branch

editorialbot commented 2 years ago

Done! branch is now joss_paper

christinahedges commented 2 years ago

@editorialbot generate pdf

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

christinahedges commented 2 years ago

Hi @reichherzerp

Thank you for addressing my comments here's a quick response:

  1. That looks greatly reduced, this will be much better for users, nice work!
  2. Great job getting the readthedocs page sorted. I have two final comments you can address after acceptance if you like. a) You probably don't need the tests in your API doc, users won't need to look up those functions on readthedocs and they don't have any API documentation. b) You could put your tutorials on your readthedocs page. You can convert them to html files using jupyter's nbconvert functionality. Having tutorials rendered on readthedocs makes them more findable online, and it also makes sure they render nicely (sometimes github's rendering of notebooks can be spotty and fickle).
  3. Not a problem on the CI, you are currently meeting the JOSS requirements which is to have tests and a demonstrated workflow to run them. CI is a big benefit, but it's probably not worth tangling with admins.

It looks like all the review criteria have been met and the submission paper seems good to me. I'll recommend we accept this.

christinahedges commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago

Paper is not ready for acceptance yet, the archive is missing

reichherzerp commented 2 years ago

@christinahedges thanks for your additional comments! Interestingly, the size further decreased (file size ~500 kb). I removed the tests and added the tutorials directly in the documentation as you proposed. I agree that it is better like that. The software is archived at 10.5281/zenodo.6573031 under the version v1.1.1. I think that I can't change this info using the editorialbot and only you can do this, right?

reichherzerp commented 2 years ago

Hello, @christinahedges do you need something more from me to finalize the review process?

danielskatz commented 2 years ago

Hi @reichherzerp - I'll jump in for a minute here on the next steps:

At this point could you:

@christinahedges can then redo the recommend-accept command and proofread the paper, if she hasn't yet done so, then the AEiC on duty that week will finish the processing.

danielskatz commented 2 years ago

Ah, now I see that you have done this - sorry to have not read the previous comments carefully enough

danielskatz commented 2 years ago

@editorialbot set v1.1.1 as version

editorialbot commented 2 years ago

Done! version is now v1.1.1

danielskatz commented 2 years ago

@editorialbot set 10.5281/zenodo.6573056 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.6573056

danielskatz commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1088/0004-637X/806/2/217 is OK
- 10.1088/1674-4527/16/10/162 is OK
- 10.3847/1538-4357/ab643b is OK
- 10.22323/1.395.0468 is OK
- 10.1098/rsif.2008.0014 is OK
- 10.1086/148912 is OK
- 10.1063/1.4928940 is OK
- 10.1063/1.4807033 is OK
- 10.1088/1475-7516/2017/06/046 is OK
- 10.1088/1475-7516/2016/05/038 is OK
- 10.5281/zenodo.5959220 is OK
- 10.5281/zenodo.5959618 is OK
- 10.22323/1.395.0978 is OK
- 10.1093/mnras/staa1650 is OK
- 10.3847/1538-4357/aa603a is OK
- 10.1088/1475-7516/2017/02/015 is OK
- 10.1086/306470 is OK
- 10.1093/mnras/staa2533 is OK
- 10.1103/PhysRevD.65.023002 is OK
- 10.1086/307452 is OK
- 10.1007/s42452-021-04891-z is OK
- 10.1016/j.physrep.2020.05.002 is OK
- 10.3847/1538-4365/ac1517 is OK
- 10.3847/2041-8213/aa6aa6 is OK
- 10.3847/1538-4357/ac2363 is OK

MISSING DOIs

- 10.1093/mnras/stac1408 may be a valid DOI for title: Anisotropic cosmic-ray diffusion in isotropic Kolmogorov turbulence

INVALID DOIs

- None
editorialbot commented 2 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/3260

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3260, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

danielskatz commented 2 years ago

@reichherzerp - there are some grammar issues in the paper that I see. I would normally create a PR for you with suggested changes, but because this is on a gitlab instance, this means I need to create a new account to do this. I will now try to do this, but I am also writing here to say that this is not something that makes it easy for people to contribute to the software.

danielskatz commented 2 years ago

Ok, now I tried to create an account but am not allowed to, I think because I don't have an email at your institution.

danielskatz commented 2 years ago

I've made my changes in a fork in https://github.com/danielskatz/proppy/commit/de54c1f6c309d367b3a296bf2cb7cb4c0bb76bc9 - I probably should be able to create a PR back to your repo, but I think this is good enough for you to see the changes I suggest. In addition, the second paragraph of the comparison section is identical to the last paragraph in the previous section. I suggest you remove one of them.

reichherzerp commented 2 years ago

@danielskatz - thank you for providing the comments on the paper. The changes are now implemented. Regarding the current problem of creating issues directly in the GitLab project, I'll check for possibilities to make this possible.

danielskatz commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...