openjournals / joss-reviews

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

[REVIEW]: SkyPy: A package for modelling the Universe #3056

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @rrjbca (Richard Rollins) Repository: https://github.com/skypyproject/skypy Version: v0.4 Editor: @arfon Reviewer: @cescalara, @rmorgan10 Archive: 10.5281/zenodo.4475347

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

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

@cescalara & @rmorgan10, 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 @arfon 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 @cescalara

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @rmorgan10

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @cescalara, @rmorgan10 it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

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

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 3 years ago
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.16 s (551.7 files/s, 43302.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          46           1029           1740           1999
reStructuredText                17            333            225            469
SVG                              1              0              0            333
YAML                            16              9              0            238
DOS Batch                        1             21              1            150
Markdown                         5             55              0            148
JSON                             1              0              0            121
make                             1             22              5            108
INI                              1             16              0             99
CSS                              1              3              0             12
TOML                             1              2              0              5
-------------------------------------------------------------------------------
SUM:                            91           1490           1971           3682
-------------------------------------------------------------------------------

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

Author                     Commits    Insertions      Deletions    % of changes
Adam Amara                       2          1497              0            6.49
Andrew R. Williamson             1             4              0            0.02
Andy Lundgren                    1            55              5            0.26
Brian Nord                       1             4              0            0.02
Ian Harrison                     8           710             84            3.44
Ian Harry                        4           246            126            1.61
JonathanDHarris                  2            72              1            0.32
Juan Pablo Cordero               3           364             19            1.66
Lucia F. de la Bella            16          1793            242            8.82
Nicolas Tessore                 70          4915           4762           41.95
Richard R                       44          3128            931           17.60
Sarah Bridle                     2            13             11            0.10
Simon Birrer                     1             0              1            0.00
nstarman                         1             3              0            0.01
philipp128                       9          1111            148            5.46
skypybot                         7             2           2819           12.23

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
Adam Amara                  276           18.4          0.0               44.57
Andy Lundgren                32           58.2         10.3               18.75
Ian Harrison                241           33.9          1.1               11.20
JonathanDHarris               3            4.2          3.2                0.00
Lucia F. de la Bella        465           25.9          8.1               19.78
Nicolas Tessore            2190           44.6          4.9               11.83
Richard R                  1239           39.6          2.9               17.68
Sarah Bridle                 11           84.6          1.1               18.18
nstarman                      3          100.0          4.0               33.33
philipp128                  307           27.6          8.7                6.84
skypybot                      1           50.0          1.3                0.00
whedon commented 3 years ago

PDF failed to compile for issue #3056 with the following error:

Can't find any papers to compile :-(

arfon commented 3 years ago

@cescalara, @rmorgan10 – This is the review thread for the paper. All of our communications will happen here from now on.

Please read the "Reviewer instructions & questions" in the first comment above.

Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

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 https://github.com/openjournals/joss-reviews/issues/3056 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

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.

arfon commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

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

rmorgan10 commented 3 years ago

Hey @rrjbca Congratulations on SkyPy! This package is awesome and is much needed in the astronomical community. Overall, the code is beautiful, the documentation is thorough, the tests are complete, and the software is easy to use. I am happy to say I only have minor suggestions coming from this review:

It would be helpful to astronomers / potential users to...

  1. produce a comprehensive list of all the things SkyPy can do. https://github.com/skypyproject/skypy/issues/443
  2. produce a guide to constructing configuration files for SkyPy. https://github.com/skypyproject/skypy/issues/444

The main motivation for these suggestions is that I think SkyPy offers a method for astronomers to make calculations by leveraging realistic distributions of redshift, mass, shape, photometry, etc. Typically, an astronomer would answer these questions empirically by querying public data from an optical survey, so SkyPy removes the data access and data querying barriers to making calculations. To avoid creating a new barrier, I think you should consider the two above suggestions as possible ways of enabling potential users to clearly see how SkyPy can factor into their analyses and how to go about the process of utilizing SkyPy.

For the two suggestions above, I have opened issues in the skypy repository with more details on how to go about implementing them. Also, here are some details for why I have left some of the reviewer check boxes empty at present:

All four of these check boxes are tied to the two issues that I opened in the skypy repository (where there are more details on how I could see going about the suggestions), and I'm looking forward to iterating with you on them. Again, congratulations on an awesome software package!

whedon commented 3 years ago

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

whedon commented 3 years ago

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

cescalara commented 3 years ago

Hi guys, I will provide some comments in the next days, just finishing up another review! Thanks for your patience.

cescalara commented 3 years ago

Hi @rrjbca, I am impressed with SkyPy. The code was easy to install, test and get working with quickly. The code itself is also carefully documented. In general, I agree with the recommendations and issues raised by @rmorgan10, and have added a comment in #433 with my thoughts. I also add some further comments/questions below.

I hope these comments are useful and am happy to discuss on any of the points. Thank you all for your work on SkyPy.

arfon commented 3 years ago

:wave: @rrjbca - just checking in to see how you're getting along here? Are you able to incorporate the feedback from @rmorgan10 and @cescalara ?

rrjbca commented 3 years ago

Hi all, yes the reviews have been incredibly helpful and we're working on incorporating everything, in particular a lot of work is going into improving our documentation. It's taking a bit longer than we anticipated but we hope to get back to you shortly.

arfon commented 3 years ago

:wave: @rrjbca – just checking in again to see how you're getting on making your updates?

rrjbca commented 3 years ago

Thank you all again for your kind and constructive comments. As a result we have now revised the manuscript and made a number of improvements to the code and documentation. We address the specific points raised in your reviews below:

@rmorgan10

@cescalara

@arfon

rrjbca commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

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

rmorgan10 commented 3 years ago

@arfon @rrjbca This looks great to me; all my suggestions have been implemented and I've checked all the boxes above for my portion of the review. Congratulations on the great effort that is SkyPy.

arfon commented 3 years ago

Great, thanks for the update @rrjbca!

@cescalara – could I ask that you come and take another look at this submission to see if you're able to check off the final checkboxes here?

arfon commented 3 years ago

In addressing the reviewers comments we have done significant work to improve the software and documentation on our development branch. Would you expect us to make a new release (nominally v0.4.1) incorporating these changes to accompany the paper?

Yes please. A new release would be great.

The references are also listed in reverse-chronological order in the bibliography. Is it possible to force these citations to be in chronological order?

I'm not sure sorry. Once we're a little closer to publishing, I'll download the paper locally and see if there's something I can do to fix this.

cescalara commented 3 years ago

Hi, great to see all the updates - I need a little more time to go through them all but I'll be in touch within the next week. Thanks for your patience.

cescalara commented 3 years ago

Hi @rrjbca, everything looks good, I really like the improved docs and new logging functionality. Thank you!

One small thing, now that I understand better the scope of SkyPy, perhaps there is also some relation to popsynth, a package that I am using in my work. In particular regarding luminosity function/density evolution simulation and the idea to have a general framework for generative modelling and stats applications. I understand that the focus for applications of SkyPy is different but there seem to be some similarities. Perhaps it could be worth mentioning for the "state of the field" bullet point. If there is really nothing out there that is related, this could also be worth mentioning explicitly!

Otherwise I accept the code and paper as is.

cescalara commented 3 years ago

Hi again @rrjbca, upon a closer look I did come across something that I'd like to check. I noticed that when sampling redshift distributions and luminosity functions (e.g. here and here) an approximation of the inverse-CDF method is used. In my experience, this is very fragile and can give biased results, with this effect depending on the size of the discretisation used.

I would recommend to replace this with simple rejection sampling for a robust approach that should be fast enough for the envisaged applications. If you prefer not to do this, then I would suggest to at least warn the user and suggest reasonable parameter ranges/resolutions for which the function has been tested, and to what precision.

Another smaller comment, the use of noise here is misleading from a pedagogical stats perspective and I would recommend to change this to something like fixed_N. It isn't really noise, just whether you are assuming that your observations arise from a poisson process.

I apologise for missing this in my earlier reviews.

arfon commented 2 years ago

:wave: @rrjbca – how are you getting on addressing this latest feedback from @cescalara?

arfon commented 2 years ago

Friendly reminder here @rrjbca to give us an update on your progress soon please.

rrjbca commented 2 years ago

Thank you for the reminder @arfon (and my apologies for missing your last message and not responding). @cescalara please find our response to your additional comments below:

rrjbca commented 2 years ago

@whedon generate pdf from branch joss-paper

whedon commented 2 years ago
Attempting PDF compilation from custom branch joss-paper. 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:

cescalara commented 2 years ago

Hi @rrjbca and @arfon! Thanks a lot for considering my comments and for your explanation. I think the changes made are very helpful and have no further requests.

arfon commented 2 years ago

⚑ thanks @rrjbca and @cescalara!

@rrjbca – At this point could you make a new release of this software that includes the changes that have resulted from this review. Then, please make an archive of the software in Zenodo/figshare/other service and update this thread with the DOI of the archive? For the Zenodo/figshare archive, please make sure that:

I can then move forward with accepting the submission.

rrjbca commented 2 years ago

Thank you all again. @arfon it might take me a couple of days to coordinate the new release but I will post here once it is ready.

rrjbca commented 2 years ago

Hi @arfon a quick question. In our .zenodo.json we currently have:

"title": "SkyPy",
"description": "A package for modelling the Universe",

When you say the zenodo title needs to match the paper title does that mean we need to change it to this:

"title": "SkyPy: A package for modelling the Universe",

or is what we have sufficient? You can see the zenodo record for our last release here

arfon commented 2 years ago

or is what we have sufficient? You can see the zenodo record for our last release here

I think what you currently have is sufficient.

arfon commented 2 years ago

@whedon set 10.5281/zenodo.4475347 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.4475347 is the archive.

arfon commented 2 years ago

@whedon recommend-accept

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

PDF failed to compile for issue #3056 with the following error:

 Can't find any papers to compile :-(
arfon commented 2 years ago

@whedon recommend-accept from branch joss-paper

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

OK DOIs

- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1016/j.ascom.2015.02.002 is OK
- 10.3847/1538-3881/aa859f is OK
- 10.1088/0264-9381/32/7/074001 is OK
- 10.3847/1538-4357/ab042c is OK
- 10.1051/0004-6361/201833880 is OK
- 10.21105/joss.03257 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon 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/2580

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2580, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true from branch joss-paper 
arfon commented 2 years ago

@whedon accept deposit=true from branch joss-paper

whedon commented 2 years ago
Doing it live! Attempting automated processing of paper acceptance...
whedon commented 2 years ago

🐦🐦🐦 πŸ‘‰ Tweet for this paper πŸ‘ˆ 🐦🐦🐦

whedon commented 2 years ago

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/2581
  2. Wait a couple of minutes, then verify that the paper DOI resolves https://doi.org/10.21105/joss.03056
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! πŸŽ‰πŸŒˆπŸ¦„πŸ’ƒπŸ‘»πŸ€˜

    Any issues? Notify your editorial technical team...

arfon commented 2 years ago

@cescalara, @rmorgan10 – many thanks for your reviews here! JOSS relies upon the volunteer effort of people like you and we simply wouldn't be able to do this without you ✨

@rrjbca – your paper is now accepted and published in JOSS :zap::rocket::boom: