search

openjournals / joss-reviews

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

[REVIEW]: Blimpy: Breakthrough Listen I/O Methods for Python #1554

Closed whedon closed 4 years ago

whedon commented 5 years ago

Submitting author: @telegraphic (Danny Price) Repository: https://github.com/UCBerkeleySETI/blimpy Version: 1.4.1 Editor: @xuanxu Reviewer: @garrettj403, @zhampel Archive: 10.5281/zenodo.3472084

Status

status

Status badge code:

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

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

@garrettj403 & @zhampel, 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 @xuanxu know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @garrettj403

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @zhampel

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

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

: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
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

zhampel commented 5 years ago

@xuanxu, @telegraphic, there are a couple of GitHub users (beyond the listed authors) who have contributed to the repo. Specifically FX196 & marksbrt.

@xuanxu are there clear guidelines on defining contributing authors. I just want to check prior to checking some boxes.

xuanxu commented 5 years ago

@zhampel authorship is a complex topic, here are some guidelines: https://joss.readthedocs.io/en/latest/review_criteria.html#authorship

the authors themselves are responsible for deciding which contributions are sufficient for co-authorship. Your job as a reviewer is to check that the list of authors appears reasonable, and if it’s not obviously complete/correct, to raise this as a question during the review.

zhampel commented 5 years ago

Yeah, it's that last phrase why I ask. I'll check off the box for now, unless the authors want to comment otherwise.

garrettj403 commented 5 years ago

Hi, sorry for the delay -- I've been really busy for the last two weeks, but I have a lot more time now. I started by taking a look at the documentation, and I'll try out the software over the next few days.

General checks

Software paper

Documentation

garrettj403 commented 5 years ago

Functionality

Currently, I'm not sure what else I can do with Blimpy. I might need to wait until there are more examples to get me started.

telegraphic commented 5 years ago

Thanks @garrettj403 (et al), we're having a look now and will get back to you soon!

zhampel commented 5 years ago

@telegraphic

After installing via pip install blimpy and running python setup.py test, I receive an error regarding the lack of module bitshuffle. Any guidance on properly prepping & running the test suite?

Regarding the docs, I'm not sure the 'Writing Docs' page is necessary nor particularly instructive for the purposes of contributing to the blimpy codebase. I think a contributing guide similar to this is more like what this requirement is for JOSS publication. I also agree with @garrettj403 that the API is rather unclear.

I think until further example usage and more in depth documentation is provided, I can't provide further review of your submission.

garrettj403 commented 5 years ago

@zhampel

They have a couple extra packages listed under extras_require in setup.py, including bitshuffle. From your local copy of blimpy, you can run: pip install -e .[full]

Again, this should be explained in the installation instructions.

xuanxu commented 5 years ago

:wave: @telegraphic: Have you had the opportunity to address the issues raised by the reviewers?

telegraphic commented 5 years ago

Hey @xuanxu , we've had a meeting about moving this forward, and I'll be devoting some time to it this weekend. Quite a few suggestions!

telegraphic commented 5 years ago

@whedon commands

whedon commented 5 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references
telegraphic commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

telegraphic commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

xuanxu commented 5 years ago

:wave: @telegraphic: How are those changes going?

telegraphic commented 5 years ago

Hi @xuanxu, getting there: I'm waiting on a pull request to be reviewed/accepted that addresses a lot of the points raised. (One of the authors just finished their PhD so took some well-deserved time off)

telegraphic commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

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

/app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:377:in parse': (tmp/1554/paper.md): mapping values are not allowed in this context at line 17 column 17 (Psych::SyntaxError) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:377:inparse_stream' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:325:in parse' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:252:inload' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:473:in block in load_file' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:472:inopen' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/psych.rb:472:in load_file' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-efe915e61673/lib/whedon.rb:115:inload_yaml' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-efe915e61673/lib/whedon.rb:85:in initialize' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-efe915e61673/lib/whedon/processor.rb:36:innew' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-efe915e61673/lib/whedon/processor.rb:36:in set_paper' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-efe915e61673/bin/whedon:55:inprepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-efe915e61673/bin/whedon:116:in <top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in `

'

telegraphic commented 4 years ago

Hmm, whedon is not happy. I just removed a single space in the metadata of paper.md, let's see if that fixes it

telegraphic commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

telegraphic commented 4 years ago

LGTM Whedon!

@xuanxu, @garrettj403 and @zhampel, thanks for your patience. We have made quite a few changes thanks to your suggestions:

1) Firstly, we have updated the paper.md with references and an expanded author list. The README is also updated, and I've added a link to our data format paper, that is now accepted to PASP (Publications of the Astronomical Society of the Pacific).

2) We removed the need for git-lfs, and now data are downloaded from our self-hosted data archive. We've also moved code coverage over from coveralls over to codecov.io, and worked on improving the coverage to 70%.

3) I added a jupyter notebook example into the new examples directory, which shows how to use the code to find the telemetry signal from the Voyager telescope from some of our data.

4) We also made a structural change to the package -- you'll notice the code has been rearranged into directories, based on what the code does (as a sidenote, apart from being logical, this makes the coverage reports much more useful!)

BEFORE:

blimpy
  |- calib_utils
  |- deprecated

AFTER

blimpy
  |- calib_utils
  |- deprecated
  |- plotting
  |- ephemeris
  |- io
garrettj403 commented 4 years ago

Hi @telegraphic

The recent changes look good. It's much easier to understand the functionality with the new changes to the paper and the workflow example.

I still have a few remaining comments that I would like to be addressed:

I'm happy to sign off once these last few issues are fixed (especially the API).

xuanxu commented 4 years ago

About the Zenodo archive: There is no need to archive the code before the review is finished. That way the archive created will include all changes made during the review process.

telegraphic commented 4 years ago

Hi @garrettj403 and @xuanxu, I've got a draft in Zenodo ready -- it looks like I can fill in the Journal ID as metadata, so I'll fill this in once the review is finished?

We've fixed the Travis-CI problem, added CONTRIBUTIONS.md, and I went through and improved docstrings. I've bumped to v 1.4.1 also, which we will push to PyPi (relevant collaborator is currently on vacation, so might take a few days).

garrettj403 commented 4 years ago

Hi @telegraphic, the changes to the docstrings look good. For some reason blimpy.waterfall.Waterfall doesn't display all of the arguments correctly online, but it does display them correctly using pydoc.

@xuanxu, I'm ready to recommend publication. The documentation is very nice and it includes an example workflow to get new users started. The package is easy to install via pip. (It's a bit harder if you want to run the unit tests, but they have included instructions in their README to tell you how to do that.) Overall, it is an interesting package that allows you to interact with Breakthrough Listen data.

xuanxu commented 4 years ago

@garrettj403 Thanks!

xuanxu commented 4 years ago

:wave: @zhampel can you review the changes made by @telegraphic and summarize what points (if any) you feel are still remaining? Thanks!

zhampel commented 4 years ago

@xuanxu I will try to get to it by the end of the week. I have had a computer failure yesterday so I can’t reasonably re-review till then.

zhampel commented 4 years ago

@telegraphic

Thanks for the reorganization of the codebase, as it's clearer how the pieces work. The example notebook is a nice addition for usage purposes. Furthermore, thanks for the clarifying the README for streamlined install, testing, usage, and some simple module calls.

@xuanxu I recommend this submission for publication.

xuanxu commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

xuanxu commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Attempting to check references...
whedon commented 4 years ago 

OK DOIs

- 10.3847/1538-4357/aa8d1b is OK
- 10.3847/2515-5172/aaa6c9 is OK
- 10.3847/2515-5172/ab010b is OK
- 10.1017/S1473550417000465 is OK
- 10.3847/1538-4357/aad005 is OK
- 10.1109/MCSE.2011.37 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK

MISSING DOIs

- None

INVALID DOIs

- None
xuanxu commented 4 years ago

@telegraphic: a couple of minor details to fix in the paper:

telegraphic commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

telegraphic commented 4 years ago

@xuanxu missing 'data' added. I think the bib style is generating 'Price, Croft et al' and 'Price, Enriquez et al', i.e. showing first two authors, to differentiate Price2019a and Price2019b?