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]: GoFish: Fishing for Line Observations in Protoplanetary Disks #1632

Closed
whedon opened this issue Aug 8, 2019 · 35 comments

Comments

@whedon
Copy link
Collaborator

commented Aug 8, 2019

Submitting author: @richteague (Richard Teague)
Repository: https://github.com/richteague/gofish
Version: v1.0.0
Editor: @arfon
Reviewer: @zhampel, @nespinoza
Archive: 10.5281/zenodo.3403894

Status

status

Status badge code:

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

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

@zhampel & @nespinoza, 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 try and complete your review in the next two weeks

Review checklist for @zhampel

Conflict of interest

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?
  • Version: Does the release version given match the GitHub release (v1.0.0)?
  • Authorship: Has the submitting author (@richteague) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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: Do the authors clearly state 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 function 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @nespinoza

Conflict of interest

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?
  • Version: Does the release version given match the GitHub release (v1.0.0)?
  • Authorship: Has the submitting author (@richteague) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

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: Do the authors clearly state 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 function 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

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

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

⭐️ 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

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

@arfon

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Re-stating here that @nespinoza won't be able to start his review until early September.

@zhampel

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

@richteague

Paper
Corrections/Clarifications:

  • requirer -> require
  • which is assumed to random -? which is assumed to be random / which is assumed random
  • Can you please specify what is meant by random, i.e. type or distribution?
  • Does 'correcting' need to be in quotes? It is a correction operation itself, no?
  • mass of the mass of the central star -> mass of the central star

Documentation

  • I recommend you provide a clearly labelled section on your GitHub README that provides some basic form of documentation & usage. Preferably also a clear link in this section that directs to your doc page. The little icon below the molecular fishing image is a bit small and easily bypassed.
  • Your readthedocs page is nice & thorough.
  • The Harvard Dataverse link directs to a page claiming that the service is unavailable. Is this a bad link or an intermittent issue?

Once access is permitted to the example data from Harvard Dataverse, I will continue my review of the codebase operability/functionality.

@labarba

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@whedon remind @nespinoza in three days

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

Reminder set for @nespinoza in three days

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 2, 2019

👋 @nespinoza, please update us on how your review is going.

@richteague

This comment has been minimized.

Copy link

commented Sep 4, 2019

@zhampel

Thanks for the initial comments!

I've corrected the manuscript draft following your suggestions. I've also updated the README to include a brief example usage and another link to the documentation. Please let me know if you feel as if there should be more here.

Regarding the data files, these should be persistent and are likely an intermittent issue. I have checked the downloads and had others verify that they're able to download the data cube so hopefully everything is good to go.

@nespinoza

This comment has been minimized.

Copy link

commented Sep 5, 2019

Hi everyone!

First of all, congrats @richteague on this very useful piece of software! I tested the whole code and all worked for me (in particular, the TWHya dataset was available from the Harvard Dataverse). I have gone over the documentation and paper, and have a few minor comments I would like to cover, and one moderate comment at the end:

Regarding the paper:

  • For some reason the references are not compiling correctly --- I think this has to do with the fact that the paper.md file in the repository is performing the citations using commas between each citation, i.e., you are doing '[@citation]' instead of simply doing [@citation]. Because of this the references are missing in the paper (and I cannot mark this above).

Regarding the code & documentation:

  • In the documentation, I had trouble viewing line [10] of the notebook (Fishing Basics --- "Extracing a Disk Averaged Spectrum") in my computer (I had to go to the actual notebook to run/see this line in my own system). This might be because the line is too long --- perhaps you could consider breaking the line in two with \.
  • A bit of a technical question: in line [12] and [13] of the notebook of the same section, you play with resampling the data. However, the errorbars go up when you bin the data in velocity-space where they should intuitively go down (you have more data on each bin, hence, errors on the flux density should go down as ~1/sqrt(n) where n is the number of samples in the bin). I checked the code and it seems this goes back to eddy, which in turn calls scipy.stats.binned_statistics. Turns out that when you ask for std in scipy.stats.binned_statistics this returns the sample standard deviation on each bin, whereas what you really want, I believe, is the error on the flux density for the given bin. Assuming independent and identically distributed data, a good measure of this is the standard error on the mean, which is given by the sample standard deviation divided by the square-root of n. I believe it is important this is reported in the documentation, because some people (like me) would perhaps be expecting the error on the flux densities to be reported and not the sample standard deviation of the bin, which are different statistics.
  • Could you please include in the documentation community guidelines? (i.e., explicitly say that the code is being actively developed in Github, how one can contribute, etc.).
@richteague

This comment has been minimized.

Copy link

commented Sep 5, 2019

Hi @nespinoza!

Thanks for the comments!

I've corrected the citation style, hopefully this works now.

I've included a line break in line 10 of the Fishing Basics notebook and appears to render OK.

I've added a small community guidelines section pointing towards the (albeit brief) contributing guide on the main repo. If you think this require more info I can update this.

Regarding the technical question, you're absolutely right. I made the choice in eddy to use just the standard deviation because the spectra will have some small levels of correlation (both spatially and spectrally). However, I've included a new argument, assume_correlated which, if set to False (the default is True), it estimates the error following the poisson statistics you described above. I've included both a note in the function docstrings to highlight this and included another cell in the notebook demonstrating this.

@nespinoza

This comment has been minimized.

Copy link

commented Sep 5, 2019

Thanks for the quick response to the comments @richteague! I'm all set then --- ready to accept the paper to JOSS (as soon as the code has a release version in Github, so I can mark the "version" part above)!

@richteague

This comment has been minimized.

Copy link

commented Sep 5, 2019

Great, thanks a lot! I'll wait to see if @zhampel has any other issues with it, then I'll update the release version on GitHub.

@zhampel

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

@richteague

Thanks for your changes, and after having been able to download the dataset, I see that the functionality is as advertised. BTW the documentation is very thorough and appreciated!

However, I have a request regarding the installation which can have an impact on those wishing to contribute (e.g. if users fork the repo and wish to make PRs). I recommend that you add an __init__.py file in the gofish subdirectory (where gofish.py lives) with the following lines:

# __init__.py

from .gofish import *

This would allow a user to (instead of running pip install gofish) clone the repo, setup a virtualenv and run python setup.py install. Then the user would have an easier time testing and contributing to your codebase. After such a change is made, I will check off the installation & contributing boxes, as this will facilitate user contributions. Then I think you can update the version. Is that acceptable?

One other thing: have you considered adding an emcee search to the advanced fishing example?

@richteague

This comment has been minimized.

Copy link

commented Sep 10, 2019

@zhampel - Thanks a lot for the report! I've added the __init__.py file to the main repo. I've thought about adding an emcee search to the documentation and this is something I want to to, however I'm working with a collaborator to get some real data that would be more appropriate for such an example. As it might take a little while to get the data, I didn't want that to delay the publication of the software.

I've also updated the version to v1.0 in GitHub (@nespinoza).

Thanks again to you both for your quick reviews!

@arfon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

@arfon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@richteague - 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:

  • The title of the archive is the same as the JOSS paper title
  • That the authors of the archive are the same as the JOSS paper authors

I can then move forward with accepting the submission.

@richteague

This comment has been minimized.

Copy link

commented Sep 10, 2019

@arfon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@richteague - minor point, could you remove Lorena's name from the authors of the Zenodo archive?

Also, could you update the title of your submission to match the paper, i.e. change it to GoFish: Fishing for Line Observations in Protoplanetary Disks

@richteague

This comment has been minimized.

Copy link

commented Sep 10, 2019

@arfon - No problem, I've made those changes!

@arfon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@whedon set 10.5281/zenodo.3403894 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

OK. 10.5281/zenodo.3403894 is the archive.

@arfon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

Attempting dry run of processing paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

Check final proof 👉 openjournals/joss-papers#960

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

@whedon accept deposit=true
@arfon

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

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

@whedon whedon added the accepted label Sep 10, 2019

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

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

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

🚨🚨🚨 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 👉 openjournals/joss-papers#961
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01632
  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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@zhampel, @nespinoza - many thanks for your reviews here

@richteague - your paper is now accepted into JOSS ⚡️🚀💥

@arfon arfon closed this Sep 10, 2019

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2019

🎉🎉🎉 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.01632/status.svg)](https://doi.org/10.21105/joss.01632)

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

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

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:

@Kevin-Mattheus-Moerman

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

@arfon this paper lacks a References heading. Should one be added?

@arfon

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

@arfon this paper lacks a References heading. Should one be added?

Good catch @Kevin-Mattheus-Moerman.

@richteague - could you please merge this small change to your paper? richteague/gofish#3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.