search

openjournals / joss-reviews

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

[REVIEW]: The Pulsar Signal Simulator: A Python package for simulating radio signal data from pulsars #2757

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @Hazboun6 (Jeffrey Hazboun) Repository: https://github.com/PsrSigSim/PsrSigSim Version: v1.0.1 Editor: @mbobra Reviewer: @JBorrow, @arjunsavel Archive: 10.5281/zenodo.4521235

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

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

@JBorrow & @arjunsavel, 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 @mbobra 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 @JBorrow

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @arjunsavel

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. @JBorrow, @arjunsavel 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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1038/217709a0 is OK
- 10.1109/MCSE.2011.37 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1007/s00159-019-0115-7 is OK
- 10.1093/nsr/nwx126 is OK
- 10.1086/181708 is OK
- 10.1086/159690 is OK
- 10.5281/zenodo.3756164 is OK
- 10.1017/pasa.2020.19 is OK

MISSING DOIs

- None

INVALID DOIs

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

mbobra commented 3 years ago

šŸ‘‹ @JBorrow @arjunsavel Thank you so much for agreeing to review this submission! You'll see your reviewer checklists above. Please feel free to ask questions here on this review thread.

Please note that this article is in collaboration with AAS publishing (the submitting authors submitted a companion paper to The Astrophysical Journal). As part of this collaboration, AAS publishing makes a small donation to the running costs of JOSS. Please take a look at the blog posts about the AAS-JOSS collaboration and JOSS' financial model. Please let me know if you have any issues with this.

JBorrow commented 3 years ago

The authors have presented a new package for the simulation and modelling of pulsar observations. It has a clean object-oriented interface, and exceptionally detailed narrative documentation and tutorial content. I found the interface particularly intuitive, and it was great (as somebody who is a 'pure' simulator) to play around with the different telescopes. I can see clearly both the scientific as well as educational benefit of such a package. As somebody not directly within the field of radio astronomy, I cannot comment on the scientific merit of the package or its results, but I will leave this to those reviewing the ApJ paper.

I have opened the following issues within the main repository:

with various concerns. Most of these are minor and should be a relatively quick fix (e.g. typos, adding a section to the paper looking at other software packages in the field). My one major concern, though, is the large amount of warnings produced when running the examples and tests. In particular, when running the test suite, I saw over 1000 warnings of various types (Runtime warnings from taking negative square roots and deprecation warnings). I would like to have these resolved before being confident in accepting this package, particularly as they are so numerous.

As an additional (not necessary) comment, I think that this package would really benefit from an integration with a symbolic unit library such as unyt, pint, or astropy.units. It appears that there has been some effort to use this within the i/o subsystem, but for the user-facing API knowing the input and output units would be really nice.

arjunsavel commented 3 years ago

I've completed my review:

With PsrSigSim, the authors have provided a well-documented and well-organized package for simulating pulsar observations. I don't specialize in radio astronomy, but to the extent of my knowledge, I believe it to be of scientific merit.

Of the issues that I have opened in PsrSigSim repository, the below are relevant for acceptance. Many are related to grammar and consistency.

For https://github.com/PsrSigSim/PsrSigSim/issues/164, a full implementation of a user-input random seed would not be necessary for acceptance; however, I would recommend briefly addressing effects of randomness on the documentation plots.

I reproduced the errors (and fixes) on installation/testing that @JBorrow reported above (running Python 3.7.6).

I also wanted to touch base about the authorship list ā€” I noticed that a few people are listed in the paper but not in AUTHORS.RST (Nathan Garver-Daniels, Maura A. McLaughlin).

Overall, the package looks great to me, and I'll be signed off pending the resolution of the above issues! Looking forward to the companion ApJ paper.

Hazboun6 commented 3 years ago

Thank you @JBorrow and @arjunsavel for your speedy reviews! We're looking over the Issues you both posted now and we'll let you know when we've been able to attend to them. Hopefully we'll be able to make most changes this week.

Hazboun6 commented 3 years ago

@mbobra One point that @JBorrow made in Issue 160 was about a reference to the AAS paper to which this work is linked. Is there a JOSS standard for how you'd like us to refer to that paper? I'm imagining it will probably take longer to appear/publish. Would you like it as .bib entry? Currently it is written as [Shapiro-Albert,2020], but I just realized that if I was following the AAS style it should be [Shapiro-Albert, In Prep], which is probably what led to some confusion.

mbobra commented 3 years ago

@Hazboun6 Good question. You don't need to cite the AAS paper. The DOI for the AAS paper will appear the JOSS paper automatically (see examples here and here; look at the left margin).

There is some documentation about the AAS publishing process process here. Here is the order of operations:

  1. The JOSS reviewers agree that the software package PsrSigSim passes review.
  2. Your paper is accepted by JOSS. However, we don't publish it yet. We pause the publication of the review pending the DOI from ApJ. Practically speaking, pausing the review means I add a label to this review thread that says "paused" and we leave this issue open.
  3. Your paper is accepted into ApJ. We the publish the JOSS paper and automatically include the ApJ DOI, which automatically closes this issue.

If you do want to specifically cite the ApJ paper in your JOSS paper, you can wait until we're at Step 3, modify your paper.md file to include the complete citation, and then we can publish the JOSS paper.

mbobra commented 3 years ago

šŸ‘‹ @Hazboun6 How is it going with the issues?

Hazboun6 commented 3 years ago

Hi @mbobra ! Thanks for checking in. We are making good progress. I think we're probably ~75% of the way through the issues. We also got the referee report from the AAS paper back and that also involved a few changes to the code, so we've been working to add that functionality in as well.

I can update the referees with which Issues have been completed within a week, and I think we'll only have a few things left at that point.

mbobra commented 3 years ago

/ooo December 14 until January 4

mbobra commented 3 years ago

šŸ‘‹ @Hazboun6 Checking in again -- how is it going? Do you need any help?

Hazboun6 commented 3 years ago

@mbobra I keep holding back on answering you thinking that I will be done with all of the changes we'd like to make. We are almost done on our end. I spent a good amount of time on the docs today and I have been closing the various Issues in our GitHub repo. I don't think we need any help at this point but I appreciate the offer. Is there a hard deadline for these responses?

mbobra commented 3 years ago

@Hazboun6 Nope, no hard deadline. I wanted to check in to make sure everything is okay. Please take all the time you need.

If you need a lot of time, that is totally fine -- but I in that case, I would pause the submission. All this does is notify the Editors in Chief that this submission will take a bit longer. Practically, all I do is put another label on this issue that says "Paused." In any case -- if we're looking at 4 weeks or less, no worries. If you think you'll need more time, I'll add the paused button (which we can unpause at any time).

Hazboun6 commented 3 years ago

@mbobra we have completed the changes to the code suggested by @JBorrow and @arjunsavel.

We branched the code and just merged all of the changes in this PR.

Thank you for your patience in how long this took us to do!

JBorrow commented 3 years ago

Happy to take another look at things soon if you think there are no other changes to be made @Hazboun6

Hazboun6 commented 3 years ago

@JBorrow please standby. I just realized that the docs did not build on readthedocs. They were of course building locally. How embarrassing... My work day starts in an hour and I'll fix ASAP. Sorry

Hazboun6 commented 3 years ago

@JBorrow and @arjunsavel We are now ready for you both to have another look!

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

arjunsavel commented 3 years ago

Hi @Hazboun6 ā€” I've completed my second pass, and I only have a few more minor comments:

Other than these, things look great!

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

Hazboun6 commented 3 years ago

@arjunsavel Thanks for the second look at the paper and docs! I've merged changes based on your suggestions and regenerated the pdf above. Also looks like the semicolon fixed the multiple citations.

JBorrow commented 3 years ago

Happy that all of my comments have been addressed. Thanks again to the authors.

One final comment to the authors, though, on their automated test suite - pyflake is very strict... You may be better off using black to format and check formatting in your tests. There's quite a lot of pyflake errors in your code base (not necessarily a bad thing) but there's no point them being there if they aren't going to be addressed.

mbobra commented 3 years ago

Thank you both so very much for your thorough reviews, @arjunsavel and @JBorrow! Your volunteer time keeps JOSS running and the JOSS team appreciates it very much ā˜€ļø

mbobra commented 3 years ago

@Hazboun6 Could you please cut a new release of PsrSigSim and deposit it in Zenodo? Please make sure that the author list on the Zenodo deposit matches the author list on the paper. Then I can move forward with accepting the paper.

Hazboun6 commented 3 years ago

@mbobra I just tagged a release and deposited in Zenodo. Should be all set. Thanks to everyone for helping make our code better!

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

mbobra commented 3 years ago

@whedon check references

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

OK DOIs

- 10.1038/217709a0 is OK
- 10.1109/MCSE.2011.37 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.1007/s00159-019-0115-7 is OK
- 10.1093/nsr/nwx126 is OK
- 10.1086/181708 is OK
- 10.1086/159690 is OK
- 10.5281/zenodo.3756164 is OK
- 10.1017/pasa.2020.19 is OK
- 10.1071/AS04022 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Hazboun6 commented 3 years ago

@mbobra do you need any info about the ApJ paper? It has been excepted and the proofs were returned yesterday, but hasn't been published yet.

mbobra commented 3 years ago

@Hazboun6 Yes! I was just about to message you with that. Please ask AAS to give you the DOI for the paper (I think they should have already generated that) and then put it in the paper.md YAML header like this. Please let me know when that's done and I can wrap up here.

mbobra commented 3 years ago

@openjournals/joss-eics - this paper is ready to publish once we have the AAS DOI.

/ cc @crawfordsm

Hazboun6 commented 3 years ago

@mbobra we finally heard back from ApJ! The DOI is now part of the paper.

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

mbobra commented 3 years ago

@whedon set 10.5281/zenodo.4521235 as doi

whedon commented 3 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
mbobra commented 3 years ago

@whedon set 10.5281/zenodo.4521235 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4521235 is the archive.

mbobra commented 3 years ago

@whedon set v1.0.1 as version

whedon commented 3 years ago

OK. v1.0.1 is the version.

mbobra commented 3 years ago

@Hazboun6 Congrats!!! All we have to do now is wait for the ApJ paper to be published. Then we can publish this one (which takes just a few minutes). Can you keep us up to date about when the ApJ paper is published?

@openjournals/joss-eics This paper, which is a AAS companion paper, is ready to publish.

arfon commented 3 years ago

@Hazboun6 - can you tell me the title of your AAS paper?

Hazboun6 commented 3 years ago

@arfon Yep! It is "A Study in Frequency-dependent Effects on Precision Pulsar Timing Parameters with the Pulsar Signal Simulator"

arfon commented 3 years ago

:+1: thanks. And do you by any chance know the AAS submission ID? (I'm assuming you don't know the DOI yet)

Hazboun6 commented 3 years ago

The DOI is 10.3847/1538-4357/abdc29 The alphanumeric past the last slash is the AAS submission ID as well. And I've put the DOI into the paper.md in the appropriate spot.