search

openjournals / joss-reviews

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

[REVIEW]: deeplenstronomy: A dataset simulation package for strong gravitational lensing #2854

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @rmorgan10 (Robert Morgan) Repository: https://github.com/deepskies/deeplenstronomy Version: v0.0.1.2 Editor: @pdebuyl Reviewer: @shreyasbapat, @jiwoncpark Archive: 10.5281/zenodo.4479712

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

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

@shreyasbapat & @jiwoncpark, 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 @pdebuyl 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 @shreyasbapat

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @jiwoncpark

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @shreyasbapat, @jiwoncpark 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
pdebuyl commented 4 years ago

@shreyasbapat @jiwoncpark make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page.

The review process will happen in this issue page, so questions to the author or to me can be added as comments here.

whedon commented 4 years ago

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

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

OK DOIs

- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.1086/301513 is OK
- 10.1103/PhysRevD.98.043526 is OK
- 10.3847/1538-4357/ab042c is OK

MISSING DOIs

- 10.25080/majora-92bf1922-00a may be a valid DOI for title: Data structures for statistical computing in python
- 10.1016/j.dark.2018.11.002 may be a valid DOI for title: Lenstronomy: multi-purpose gravitational lens modelling software package

INVALID DOIs

- None
shreyasbapat commented 4 years ago

There are a few DOIs missing !

rmorgan10 commented 4 years ago

There are a few DOIs missing !

The pandas DOI has been added, and the lenstronomy citation has been updated to the journal article (including the DOI) instead of ArXiv.

https://github.com/deepskies/deeplenstronomy/commit/e9ae93b79e4e405f27dba00dbebbc535ac2472a3

pdebuyl commented 4 years ago

@whedon check references

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

OK DOIs

- 10.1038/s41586-020-2649-2 is OK
- 10.25080/majora-92bf1922-00a is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1016/j.dark.2018.11.002 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.1086/301513 is OK
- 10.1103/PhysRevD.98.043526 is OK
- 10.3847/1538-4357/ab042c is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

:wave: @jiwoncpark, please update us on how your review is going.

whedon commented 4 years ago

:wave: @shreyasbapat, please update us on how your review is going.

pdebuyl commented 4 years ago

@jiwoncpark @shreyasbapat gentle reminder

jiwoncpark commented 4 years ago

@pdebuyl Apologies for the delay in my update. I should be finished with my review by tomorrow.

pdebuyl commented 4 years ago

Thank you for the reply @jiwoncpark . Since the spring, the timeframe for a JOSS review is 6 weeks maximum. I update the review regularly to make sure that the review will happen but there is no emergency for tomorrow.

pdebuyl commented 3 years ago

@shreyasbapat @jiwoncpark gentle reminder

There are several ways to proceed with the review. You can check the items and proceed with direct feedback to the author or prepare a bulk review. Either way is fine, feel free to ask questions here about it.

rmorgan10 commented 3 years ago

@pdebuyl I would like to put the deeplenstronomy paper on arXiv at some point, but this is the first time I've worked with JOSS. Would you recommend waiting until after the review process for any reason?

pdebuyl commented 3 years ago

Hi @rmorgan10 I would recommend waiting for the publication here to save you the trouble of updating the arXiv version. You are free to proceed however (see https://joss.readthedocs.io/en/latest/submitting.html?highlight=arxiv#preprint-policy ).

Just make sure to not reference the paper as published in joss yet (mention of submission is fine of course).

rmorgan10 commented 3 years ago

@pdebuyl If I wait until after the review process (and deeplenstronomy is accepted), does JOSS release the TeX for the paper to me?

pdebuyl commented 3 years ago

No. Officially, JOSS only sees the input .md file and the pdf output. It is possible to execute our code locally of course. It uses pandoc. It is also possible to use pandoc directly, provided you have the template and logo. I can help if you wish (not anymore today however).

arfon commented 3 years ago

@rmorgan10 - you might be able to make a TeX version using this script: https://github.com/mattpitkin/psrqpy/blob/master/paper/Makefile

pdebuyl commented 3 years ago

@shreyasbapat @jiwoncpark any news on this?

shreyasbapat commented 3 years ago

I promise to review this in next 2 days😀

jiwoncpark commented 3 years ago

Hi @rmorgan10, I created an issue at the deeplenstronomy repo to address some of the review checklist items, including tests and function docstrings. Separately, I'm adding my comments on the paper manuscript below. @pdebuyl Please let me know if the issue or paper comments should be placed elsewhere.

Paper comments

Background

Statement of need

Summary

State of the field Currently, there is no section or paragraph describing the "state of the field" (addressing the review criterion: "Do the authors describe how this software compares to other commonly-used packages?"). I'm not sure if this part of the JOSS requirements, but if I can mention my own software package, baobab (https://github.com/jiwoncpark/baobab) is a very similar lenstronomy wrapper with a focus on lens modeling. It was used to generate the training set for two modeling papers, Park et al 2020 and Wagner-Carena et al 2020. deeplenstronomy seems more general.

Overall style comments

pdebuyl commented 3 years ago

Thank you @jiwoncpark for the review! The issue and comments are at the right place. There is some freedom with respect to how the review is conducted, as long as I see the progress on this issue page.

pdebuyl commented 3 years ago

@rmorgan10 there is some confusion related to the link error mentioned in https://github.com/deepskies/deeplenstronomy/issues/41 : you have a repository called deeplenstronomy with the notebooks. Is that repository supposed to remain online? If so, could you rename it?

pdebuyl commented 3 years ago

Edit/PS to previous comment: "you have a repository called deeplenstronomy in your github profile"

rmorgan10 commented 3 years ago

Edit/PS to previous comment: "you have a repository called deeplenstronomy in your github profile"

Yeah that was a staging area for the notebooks before we enabled github-pages for the deepskies/deeplenstronomy repo. I should really just delete the personal repo. Thanks for pointing that out!

rmorgan10 commented 3 years ago

@jiwoncpark Thanks for your review! I think you've pointed out things that will help improve deeplenstronomy a lot. I've started working on some of the small things in the issue you opened in the deeplenstronomy repo.

I just wanted to give everyone a heads up that I won't be focusing too much on implementing major changes from the JOSS reviews until I have some more free time (which for me will start Jan 5). So @shreyasbapat no need to rush to get your review in if you are also busy.

Thanks again!

pdebuyl commented 3 years ago

Hi @rmorgan10 thanks for the heads up. I'll check back in two weeks as well.

pdebuyl commented 3 years ago

@rmorgan10 gentle reminder

rmorgan10 commented 3 years ago

Hi @pdebuyl, thanks for the ping. I'll be wrapping up my responses @jiwoncpark 's review this week. I believe I'm still waiting on @shreyasbapat 's review though. If possible, I'd like to work on them simultaneously, so @shreyasbapat do you have an estimate for when your comments will be ready?

pdebuyl commented 3 years ago

Thanks for the reply @rmorgan10 . Indeed, you can wait for the second review (JOSS allows for both ways of working).

pdebuyl commented 3 years ago

@shreyasbapat gentle reminder for the review.

The review page was opened more than 6 weeks ago, so it would be nice to have the review soon.

shreyasbapat commented 3 years ago

I think @jiwoncpark mentioned a majority of issues already. I created an issue which might not be very imporant from JOSS persective, but from a user's and maintainability perspective.

I feel once the tests and documentation is done, we would be good to go.

https://github.com/deepskies/deeplenstronomy/issues/42

pdebuyl commented 3 years ago

Hi @shreyasbapat thank you for the feedback.

pdebuyl commented 3 years ago

@rmorgan10 many checkboxes have been ticked by @shreyasbapat now.

I suggest that you take into account all the "non paper" feedback so far. @shreyasbapat , your comments on the paper will be needed as well.

rmorgan10 commented 3 years ago

Hi All, thanks for the reviews! If I understand correctly, it looks like the major required improvements are:

  1. Making the doc strings available via sphinx
  2. Organize the tests and clarify what is being tested
  3. Minor edits to the paper text

I am happy to do all three and will hopefully have all issues resolved at the end of next week if not sooner. I am glad the code and its functionalities were satisfactory. Thanks again for your careful reviews of deeplenstronomy!

pdebuyl commented 3 years ago

@whedon generate pdf

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:

pdebuyl commented 3 years ago
  • A code snippet and a short yaml file would be helpful, as a preview of the API.

Including such content in the paper goes against the guidelines, we consider it as documentation and recommend its inclusion in the proper documentation of the package.

rmorgan10 commented 3 years ago

Thanks @pdebuyl . Code snippets and yaml file excerpts are available in the provided example notebooks, which are referenced in the paper, so hopefully this comment is already addressed within the JOSS guidelines.

https://deepskies.github.io/deeplenstronomy/Notebooks/

rmorgan10 commented 3 years ago

Hi All,

I just wanted to give an update that I've finished responding to the initial reviews. Please let me know when you have feedback for my responses

pdebuyl commented 3 years ago

Thank you for the notification @rmorgan10

@shreyasbapat @jiwoncpark can you go over the changes and update the opened issues at deeplenstronomy's repo and the checkboxes here?

shreyasbapat commented 3 years ago

Looks good from my side

pdebuyl commented 3 years ago

Hi @shreyasbapat thanks for finishing the review!

Can the issue https://github.com/deepskies/deeplenstronomy/issues/42 be closed?

jiwoncpark commented 3 years ago

The text and code look really good to me, @rmorgan10! I've checked off the boxes. Super minor suggestions/comments:

Once again, congrats on a very nice package!

rmorgan10 commented 3 years ago

Thanks @jiwoncpark! I've implemented your line 43 changes. Please let me know if there is a better citation for SLSprinkler or baobab. I am considering setting up GitHub actions for CI at some point, but that will be slightly down the road.

pdebuyl commented 3 years ago

Thank you @jiwoncpark for the review!

@rmorgan10 I'll now proceed to some editorial checks before handing out the paper to the editor-in-chief (see https://joss.readthedocs.io/en/latest/editing.html#after-reviewers-recommend-acceptance ).

pdebuyl commented 3 years ago

@whedon check references

pdebuyl commented 3 years ago

@whedon generate pdf

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

OK DOIs

- 10.1038/s41586-020-2649-2 is OK
- 10.25080/majora-92bf1922-00a is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1016/j.dark.2018.11.002 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.1086/301513 is OK
- 10.1103/PhysRevD.98.043526 is OK
- 10.3847/1538-4357/ab042c is OK

MISSING DOIs

- 10.1163/1872-9037_afco_asc_3175 may be a valid DOI for title: baobab

INVALID DOIs

- None