[REVIEW]: OctApps: a library of Octave functions for continuous gravitational-wave data analysis

whedon commented 6 years ago

Submitting author: @kwwette (Karl Wette) Repository: https://github.com/octapps/octapps Version: v0.1 Editor: @danielskatz Reviewer: @stevenrbrandt Archive: 10.5281/zenodo.1283525

Status

Status badge code:

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

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) 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

@stevenrbrandt, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @danielskatz know.

Review checklist for @stevenrbrandt

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v0.1)?
[x] Authorship: Has the submitting author (@kwwette) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] 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

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

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @stevenrbrandt it looks like you're currently assigned as the reviewer for 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:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

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 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

danielskatz commented 6 years ago

👋 @stevenrbrandt - thanks for agreeing to do this. Since you've done one of these before, I'll just let you get to it. Let me know if you have any questions or problems.

stevenrbrandt commented 6 years ago

@kwwette, I got stuck at the "make check" phase above (though I had problems with installation because of LALSuite).

CostFunctionsBinary.m: 
CostFunctionsBinary.m:   tol = -1e-2;
CostFunctionsBinary.m:   assert ( sol_v2.Nseg, 36, tol );
CostFunctionsBinary.m:   assert ( sol_v2.Tseg, 864000, tol );
CostFunctionsBinary.m:   assert ( sol_v2.mCoh, 0.800740, tol );
CostFunctionsBinary.m:   assert ( sol_v2.mInc, 0.043971, tol );
CostFunctionsBinary.m: !!!!! test failed
CostFunctionsBinary.m: exception encountered in Fortran subroutine xgammainc_
CostFunctionsBinary.m: -------------------------------------------------
Makefile:233: recipe for target 'CostFunctionsBinary.m.test' failed
make[1]: *** [CostFunctionsBinary.m.test] Error 1
make[1]: Leaving directory '/octapps'
Makefile:186: recipe for target 'check' failed
make: *** [check] Error 1

I attempted to use your code from within Docker. I used Debian jessie because that was what the instructions on the LALSuite site said to do.

Here's my install script:

FROM debian:jessie
RUN apt-get update
RUN apt-get install -y git octave liboctave-dev swig3.0 texinfo 
RUN echo deb http://www.deb-multimedia.org jessie main non-free >> /etc/apt/sources.list
RUN echo deb-src http://www.deb-multimedia.org jessie main non-free >> /etc/apt/sources.list
RUN echo deb http://httpredir.debian.org/debian/ jessie-backports main >> /etc/apt/sources.list
RUN apt-get update
RUN apt-get install --force-yes -y deb-multimedia-keyring
RUN apt-get update
RUN apt-get install -y ffmpeg libgsl-dev 
RUN echo deb http://software.ligo.org/lscsoft/debian jessie contrib >> /etc/apt/sources.list
RUN echo deb-src http://software.ligo.org/lscsoft/debian jessie contrib >> /etc/apt/sources.list
RUN apt-get update
RUN apt-get install --force-yes -y lscsoft-archive-keyring
RUN apt-get update # This step missing from LAL docs, as was "force-yes" above
RUN apt-get install -y lal lal-octave
RUN git clone https://github.com/octapps/octapps.git
RUN apt-get install -y build-essential
WORKDIR /octapps
RUN make
RUN echo source /octapps/octapps-user-env.sh >> /root/.bashrc
RUN make check

kwwette commented 6 years ago

Should be fixed in PR https://github.com/octapps/octapps/pull/13

@whedon generate pdf

kwwette commented 6 years ago

@whedon commands

whedon commented 6 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

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

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

kwwette commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

danielskatz commented 6 years ago

@stevenrbrandt - I think this is back to you now.

stevenrbrandt commented 6 years ago

@kwwette I'm looking for examples and community guidelines for contributing above. I'm having trouble finding these two things. Can you help? Thanks.

kwwette commented 6 years ago

@stevenrbrandt Examples: each Octave function has embedded tests which give some ideas as to usage. I've now updated the HTML documentation at https://octapps.github.io/ to include those examples as part of the HTML documentation. See e.g. https://octapps.github.io/cw_002doptimal_002dsearch_002dsetup.html#cw_002doptimal_002dsearch_002dsetup.

There are brief guidelines for contributing in the README.md, pointing to the OctApps issue tracker on GitHub for posting issues/bugs and submitted patches for fixes/new features.

danielskatz commented 6 years ago

@stevenrbrandt - This is back to you

stevenrbrandt commented 6 years ago

The current Dockerfile builds but does not run

bash: /octapps/octapps-user-env.sh: No such file or directory
root@e9b061c40f66:/tmp/octapps# find / -name octapps-user-env.sh
/tmp/octapps/octapps-user-env.sh

That should be easy to fix.

stevenrbrandt commented 6 years ago

It would be nice if the examples present in, e.g. https://octapps.github.io/cw_002doptimal_002dsearch_002dsetup.html#cw_002doptimal_002dsearch_002dsetup came with some text describing what they are examples of and what the expected output is.

stevenrbrandt commented 6 years ago

While the reference guide and examples seem reasonable, I would like to see a higher level introduction to how to use the software. I don't think a tutorial is required by JOSS, but it would be nice to have. This is the sort of thing I would need to check off the "Functionality documentation" check box.

kwwette commented 6 years ago

@stevenrbrandt : Re. Dockerfile - thanks for spotting that, this should be fixed in the repository shortly.

Re. examples - I'll have a look at adding some more explanatory text to at least some of the examples, e.g. the ones that are gravitational-wave research focused, like the ones you referenced. I'm not sure I'll be able to do this for all the functions in OctApps though.

Re. the tutorial - I'll have to have a think about this. Within its specific scientific research area, OctApps is quite a general package which can be used to answer different scientific questions, so i.e. there's not a singular usage case which would be appropriate for a tutorial. The examples in many of the codes are meant to show how to answer different questions in gravitational-wave research; one of these could be expanded into a more complete worked example. I'm not sure I could do this for all the research-focused functions in OctApps however.

/cc @ReinhardPrix (co-author on this paper) - any ideas for a problem / material we could use for an OctApps tutorial / worked example?

kwwette commented 6 years ago

Dockerfile fixed in https://github.com/octapps/octapps/commit/57df565b0119d1312b56f370337aab7ebcbbc65b

dbkeitel commented 6 years ago

@kwwette @ReinhardPrix : Sensitivity depth estimation would be a nice case for a tutorial, as it could be useful for a wider user group at least within Continuous Waves.

Though I guess with "a higher level introduction to how to use the software" @stevenrbrandt might have rather meant things like links to general octave documentation/tutorials, a description of often-used data structures within our package (if any, iirc only the histogram class is shared widely between functions?), etc...? Or at least I'd personally think that would be the most appropriate kind of high-level docs for a package that's more a collection of small tools than a single tool for a specific purpose.

stevenrbrandt commented 6 years ago

@kwwette ping

kwwette commented 6 years ago

@stevenrbrandt - I will try to get back to this this week

kwwette commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

kwwette commented 6 years ago

@stevenrbrandt @danielskatz Apologies for the delay. I have now added a tutorials section to the OctApps reference manual: see https://octapps.github.io/Tutorials.html. There are tutorials on two general-purpose features of OctApps, and several tutorials on the more research-specific functions which illustrate OctApps's usage in answering research questions in continuous gravitational-wave research.

For more general Octave tutorials, I'd recommend starting with the Octave manual at https://octave.org/doc/v4.4.0/.

I hope this is enough additional material to complete the OctApps review. Please let me know if you have any further questions.

kwwette commented 6 years ago

@danielskatz @arfon I have a question regarding JOSS copyright policy: does JOSS permit the final PDF version of journal articles to be cross-posted to arXiv (https://arxiv.org/)? Thanks!

danielskatz commented 6 years ago

@stevenrbrandt - back to you

danielskatz commented 6 years ago

@kwwette - yes, the paper is CC-BY licensed by you, so you can do whatever else you want with it.

stevenrbrandt commented 6 years ago

OK, I think things look good now. @danielskatz shall I close this issue?

kwwette commented 6 years ago

@stevenrbrandt - Thank you very much for the review!

@danielskatz - Before formally accepting the paper I have some minor changes I'd like to make to the paper text (adding references to funding agencies and some fixes to author affiliations). I'll make these in the next ~24 hours, and let you know once the paper has been updated. Thanks!

danielskatz commented 6 years ago

when you both are satisfied, please let me know (but don't close the issue)

kwwette commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

kwwette commented 6 years ago

@danielskatz - I have now made the minor changes to the paper, and am happy to move forward with the publication process. I have also made a v0.2 release of OctApps, in light of the changes that have been made during this review - if possible it would be good to use that version number (v0.2) for the JOSS paper.

@stevenrbrandt - Back to you to confirm your sign-off. Thanks again!

kwwette commented 6 years ago

@danielskatz - I've also created an archive on Zenodo for version 0.2, the DOI is 10.5281/zenodo.1283525

danielskatz commented 6 years ago

@stevenrbrandt - Please confirm your sign-off

stevenrbrandt commented 6 years ago

@danielskatz I confirm.

danielskatz commented 6 years ago

👋 @arfon - over to you.

arfon commented 6 years ago

@whedon set 10.5281/zenodo.1283525 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1283525 is the archive.

arfon commented 6 years ago

@stevenrbrandt - many thanks for your review here and to @danielskatz for editing this submission ✨

@kwwette - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00707 :zap: :rocket: :boom:

whedon commented 6 years ago

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

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

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

This is how it will look in your documentation:

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:

Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=

kwwette commented 6 years ago

@arfon @danielskatz @stevenrbrandt - Excellent, thank you!

ReinhardPrix commented 6 years ago

That's wonderful, thanks a lot to the reviewers, and congrats to all co-authors, especially to Karl who has done all the hard work of getting this paper together and published!

openjournals / joss-reviews