Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
40 tasks done
whedon opened this issue Feb 23, 2021 · 51 comments
Closed
40 tasks done

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

whedon opened this issue Feb 23, 2021 · 51 comments
Assignees
Labels
accepted published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Feb 23, 2021

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

⚠️ JOSS reduced service mode ⚠️

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

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@rrjbca) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @rmorgan10

Conflict of interest

  • I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@rrjbca) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
@whedon
Copy link
Author

whedon commented Feb 23, 2021

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

⚠️ JOSS reduced service mode ⚠️

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.

⭐ Important ⭐

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
Copy link
Author

whedon commented Feb 23, 2021

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

This comment has been minimized.

@arfon
Copy link
Member

arfon commented Feb 23, 2021

@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 #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
Copy link
Member

arfon commented Feb 23, 2021

@whedon generate pdf from branch joss-paper

@whedon
Copy link
Author

whedon commented Feb 23, 2021

Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Feb 23, 2021

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@rmorgan10
Copy link

rmorgan10 commented Mar 2, 2021

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. Comprehensive List of Features - JOSS Review skypyproject/skypy#443
  2. produce a guide to constructing configuration files for SkyPy. Guide to Constructing Configuration Files - JOSS Review skypyproject/skypy#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:

  • Functionality; Functionality: The claims as I understand them are functionality to sample realistic distributions and a pipeline for doing the actual sampling. I would like the specific functionalities (shape, luminosity, redshifit, etc.) to be stated (in the paper and in the documentation) so that the functional claims are well defined for SkyPy to meet them.
  • Documentation; Example usage: The documentation includes examples, but I would like to see an example for how to go about creating a configuration file to better satisfy the "how to use the software" requirement
  • Documentation; Functionality documentation: The API methods all have fantastic documentation, but I would like to see a comprehensive list of features included in the documentation so that users can determine whether SkyPy will be useful for them in a quick glance.
  • Software Paper; Summary: I would say the current "high-level" description is one step too high. You mention the sampling of realistic distributions, but it would be better to list the exact physical quantities involved in SkyPy, thus giving a more specific but still high-level overview of SkyPy's utility.

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
Copy link
Author

whedon commented Mar 9, 2021

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

@whedon
Copy link
Author

whedon commented Mar 9, 2021

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

@cescalara
Copy link

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

@cescalara
Copy link

cescalara commented Mar 14, 2021

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.

  • Can you clarify the contributions of co-authors who have minor/no contributions to the code?
  • As I understand, at the moment SkyPy focusses on simulating observed galaxy properties as seen by different detectors. Are there plans to include the simulation of undetected objects and stochastic selection effects?
  • All the examples run smoothly, but it would be nice to have some optional logging to show the user what the code is up to. This would be helpful for users designing and debugging their own pipelines.
  • It seems that there is a small typo or bug on this page in the docs. When I run $ skypy examples/galaxies/sdss_photometry.yml sdss_photometry.fits I get the following error skypy: error: unrecognized arguments: sdss_photometry.fits.
  • Can you clarify the relationship to other packages with similar functionality/goals? I am aware of the following python packages which seem to have some overlap in scope (correct me if I am wrong).

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
Copy link
Member

arfon commented May 14, 2021

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

@rrjbca
Copy link

rrjbca commented May 17, 2021

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
Copy link
Member

arfon commented Jun 27, 2021

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

@rrjbca
Copy link

rrjbca commented Jun 30, 2021

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

  • In PR #456 we added a Feature List to the documentation outlining the galaxy properties that we implement models for in the library. We also added a second paragraph in the “Summary” section of the manuscript to include the high-level description of the package’s functionality.
  • In PR #454 we added a Configuration Files section to the documentation demonstrating and explaining how to construct configuration files.
  • In PR #457 we clarify the documentation for our Galaxy Spectrum and Photometry modules stating that we use speclite for our photometric calculations and that users should refer to their documentation for the list of available filters and instructions to use your own filters.

@cescalara

  • The list of co-authors on the paper includes all members of the SkyPy Collaboration, each of whom has directly contributed to the preparation of the manuscript. Those with no commits to the repository have still made important contributions to the software in areas such as planning, design, management and review which we believe are important to recognise.
  • Currently SkyPy can simulate galaxies down to a given apparent magnitude or stellar-mass limit. It is up to the user how they wish to interpret the output of these functions. For example, a user could naively use SkyPy to simulate an “observed” noise-free magnitude-limited sample. They could also simulate a deeper sample and apply their own empirical selection, or use the sample to generate their own image simulations. These additional processing steps are not explicitly implemented within SkyPy, but the modular design allows users to integrate external software implementing these process into a SkyPy pipeline.
  • Thank you for the suggestion, this has now been implemented in PR #453 and can be run in the development version using the command line flag --verbose / -v to log function calls and file I/O.
  • It looks like you have installed the most recent stable version 0.4 but are following instructions in the documentation for the development branch. Could you please try again following the instructions in the v0.4 documentation? Specifically you should run skypy sdss_photometry.yml --format fits
  • The v0.4 SkyPy release currently does not implement either image simulations or models for the galaxy-halo connection so there is not currently any notable overlap. However, as we develop new features we will continue to reuse existing high-quality software where possible, preferring to write interfaces between the SkyPy Pipeline infrastructure and libraries such as GalSim and Halotools. We added an extra line at the end of the Summary section in the manuscript to clarify this point.

@arfon

  • 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?
  • The following citation macro in markdown [@Astropy2013; @Astropy2018] leads to the following citation in the manuscript: “(Astropy Collaboration, 2018, 2013)”. The references are also listed in reverse-chronological order in the bibliography. Is it possible to force these citations to be in chronological order?

@rrjbca
Copy link

rrjbca commented Jun 30, 2021

@whedon generate pdf from branch joss-paper

@whedon
Copy link
Author

whedon commented Jun 30, 2021

Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Jun 30, 2021

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@rmorgan10
Copy link

@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
Copy link
Member

arfon commented Sep 10, 2021

@whedon set 10.5281/zenodo.4475347 as archive

@whedon
Copy link
Author

whedon commented Sep 10, 2021

OK. 10.5281/zenodo.4475347 is the archive.

@arfon
Copy link
Member

arfon commented Sep 10, 2021

@whedon recommend-accept

@whedon whedon added the recommend-accept Papers recommended for acceptance in JOSS. label Sep 10, 2021
@whedon
Copy link
Author

whedon commented Sep 10, 2021

Attempting dry run of processing paper acceptance...

@whedon
Copy link
Author

whedon commented Sep 10, 2021

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

 Can't find any papers to compile :-(

@arfon
Copy link
Member

arfon commented Sep 10, 2021

@whedon recommend-accept from branch joss-paper

@whedon
Copy link
Author

whedon commented Sep 10, 2021

Attempting dry run of processing paper acceptance...

@whedon
Copy link
Author

whedon commented Sep 10, 2021

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
Copy link
Author

whedon commented Sep 10, 2021

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

Check final proof 👉 openjournals/joss-papers#2580

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#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
Copy link
Member

arfon commented Sep 10, 2021

@whedon accept deposit=true from branch joss-paper

@whedon whedon added accepted published Papers published in JOSS labels Sep 10, 2021
@whedon
Copy link
Author

whedon commented Sep 10, 2021

Doing it live! Attempting automated processing of paper acceptance...

@whedon
Copy link
Author

whedon commented Sep 10, 2021

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@whedon
Copy link
Author

whedon commented Sep 10, 2021

🚨🚨🚨 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 👉 Creating pull request for 10.21105.joss.03056 joss-papers#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
Copy link
Member

arfon commented Sep 10, 2021

@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 ⚡🚀💥

@arfon arfon closed this as completed Sep 10, 2021
@whedon
Copy link
Author

whedon commented Sep 10, 2021

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.03056/status.svg)](https://doi.org/10.21105/joss.03056)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.03056">
  <img src="https://joss.theoj.org/papers/10.21105/joss.03056/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.03056/status.svg
   :target: https://doi.org/10.21105/joss.03056

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS Python recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

5 participants