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]: PyWavelets: A Python package for wavelet analysis #1237

Closed
whedon opened this Issue Feb 5, 2019 · 92 comments

Comments

Projects
None yet
8 participants
@whedon
Copy link
Collaborator

whedon commented Feb 5, 2019

Submitting author: @grlee77 (Gregory Lee)
Repository: https://github.com/PyWavelets/pywt
Version: v1.0.3
Editor: @jedbrown
Reviewer: @rafat, @souopgui
Archive: 10.5281/zenodo.2634243

Status

status

Status badge code:

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

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

@rafat & @souopgui, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @jedbrown know.

Please try and complete your review in the next two weeks

Review checklist for @rafat

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: v1.0.3
  • Authorship: Has the submitting author (@grlee77) 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 @souopgui

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: v1.0.3
  • Authorship: Has the submitting author (@grlee77) 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

whedon commented Feb 5, 2019

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @rafat, it looks like you're currently assigned as the reviewer for 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
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 5, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 5, 2019

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Feb 5, 2019

@rafat @souopgui 👋 Welcome and thanks for agreeing to review! The comments from @whedon above outline the review process, which takes place in this thread (possibly with issues filed in the PyWavelets repository). I'll be watching this thread if you have any questions.

@grlee77 Can you clarify whether this review should take place on the 'master' branch (where your paper sources currently reside) or in the '1.0.x' branch which contains the stated version? Thanks.

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Feb 7, 2019

The features and functionality are the same in both 1.0.1 and master so I am fine with reviewing based on 1.0.1. The main difference is that master has dropped Python 2 support and has a few minor fixes. We will eventually include the fixes in a future release from the 1.0.x branch as well.

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Feb 7, 2019

There are a number of basic demos of usage in the demo folder of the repository. The API documentation is pretty complete, but the docs don't have a lot of tutorials/demos. Some concrete examples for scientific applications are provided below.

There are a some image denoising demos functions in scikit-image that rely on PyWavelets (specifically for skimage.restoration.estimate_sigma and skimage.restoration.denoise_wavelet):
http://scikit-image.org/docs/dev/auto_examples/filters/plot_cycle_spinning.html
http://scikit-image.org/docs/dev/auto_examples/filters/plot_denoise_wavelet.html

An example of wrapping the wavelet transform into an operator (e.g. for potential use as a sparsity-enforcing regularizer in iterative medical image reconstruction) is available within ODL:
https://github.com/odlgroup/odl/blob/master/examples/trafos/wavelet_trafo.py

For time series analysis, a user working in the field of geophysics recently posted the following notebook with some examples to the mailing list:
https://gist.github.com/aprovecharLab/09ded80ae59ea510334e00926a15920f

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Feb 7, 2019

Thanks, @grlee77. Note that upon acceptance, we'll ask you to tag and archive a release that includes any revisions that may have resulted from the review. That archive should include the final sources for the paper, which are currently in 'master'.

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Feb 7, 2019

Okay, in that case, please just review from master and we will tag a new release once the review is completed. The 1.0.x branch was created to allow maintenance/bug fixes releases for Python 2.7 support through 2020 or so, but any new features introduced will not be backported to that branch.

@rafat

This comment has been minimized.

Copy link
Collaborator

rafat commented Feb 15, 2019

@grlee77 There are a few minor issues with .rst files in the doc/source folder that are causing test_doc.py to fail when I run it manually from the folder. For example, line 335 of expected value has "Short name: db" but the code correctly returns "Short name: gaus" . There are no issues with the Python code as far as can I tell but you may want to take a look at the .rst files.

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Feb 15, 2019

Thanks for pointing that out. It seems the tests in that file were being skipped by our CI scripts. I have fixed the failures and made sure TravisCI is running these tests in PyWavelets/pywt#463.

@souopgui

This comment has been minimized.

Copy link
Collaborator

souopgui commented Feb 17, 2019

@grlee77 Quick questions to help me finalize my review:

  1. Apart from the references to Mallat "A Theory for Multiresolution Signal Decomposition: The Wavelet Representation", Wickerhauser "Adapted Wavelet Analysis from Theory to Software" and the thesis that started this work, is there other part of the doc that I am missing? I think that for a topic like the wavelet, there should be a little more documentations, think of Haar, Morlet, Meyer, coifman, Daubechies, etc. ?
  2. Is there a way to try the automated test outside of the install procedure?
@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Feb 18, 2019

  1. is there other part of the doc that I am missing?

I think you are asking about the PyWavelets documentation and not the manuscript itself? Specific functions do have additional references to the literature (see for example the docs for threshold or fswavedecn, but I agree that we are lacking an overview page with basic theory and key references, etc. Wavelets are well covered in many textbooks at this point, but it might still be worth having an abbreviated summary in the PyWavelet docs.

  1. Is there a way to try the automated test outside of the install procedure?

Yes, try:

import pywt
pywt.test()
@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Mar 13, 2019

hey @rafat, just checking if you are still working on this and if there are any other items left to address?

I just opened a PR at PyWavelets/pywt#475 to add a reference to the book by Daubechies, more clearly link Mallat to the multiresolution property of the transform, and to add a brief discussion of Kymatio in relation to PyWavelets.

@rafat

This comment has been minimized.

Copy link
Collaborator

rafat commented Mar 13, 2019

Hi @grlee77 @jedbrown I finished my review a couple of weeks ago. Pywavelets is a very good software and everything works as it should on two different systems (Windows and Linux).

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Mar 14, 2019

Thanks, @rafat.

@souopgui How's your review going? It looks like there are just a few items left.

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Mar 14, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Mar 14, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Mar 14, 2019

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Apr 4, 2019

Just wanted to check in with @souopgui as the review seems to have stalled: Are there still remaining items to address?

I think the only substantial change in master made subsequent to the review comments was updating the underlying testing framework used from nose to pytest since nose is no longer being updated. The tests themselves are the same as before (but hopefully will be refactored to some extent in the future to also make better use of features like pytest.mark.parametrize).

@souopgui

This comment has been minimized.

Copy link
Collaborator

souopgui commented Apr 5, 2019

@grlee77 , I have not paid attention to email coming from github lately.
The toolbox is great and I am already recommending people to give it a try. For a topic of the importance of the wavelet, a toolbox like this one was missing. With the implementation of all the filters you guys implemented, this is a great tools.
With that, I am done with my review. Being less familiar with the review on github, if I am missing something to close the process, please let me know. And sorry a again for being out of the loop for this long.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 5, 2019

@souopgui There are points unchecked in your review checklist at the top of this issue. Can you confirm those points so we can move forward with acceptance? Thanks!

@souopgui

This comment has been minimized.

Copy link
Collaborator

souopgui commented Apr 6, 2019

@jedbrown I just checked those points based on the responses to my questions from the authors. I fully recommend the software.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 6, 2019

@whedon check references

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 6, 2019

Attempting to check references...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 6, 2019


OK DOIs

- 10.5281/zenodo.1407172 is OK
- 10.1137/1.9781611970104 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.7717/peerj.453 is OK
- 10.5281/zenodo.1442734 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 6, 2019

@grlee77 There is one citation Stephan Mallat [@mallat2008wavelet] that is "in-text" and should be written Stephan Mallat [-@mallat2008wavelet] or @mallat2008wavelet. See https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html#citation_syntax for syntax. That's all I notice so I think we'll be ready to archive after that one minor fix.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 11, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
100 16 0 16 0 0 319 0 --:--:-- --:--:-- --:--:-- 326
sh: 44: Syntax error: newline unexpected
Looks like we failed to compile the Crossref XML

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019


OK DOIs

- 10.1137/1.9781611970104 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.7717/peerj.453 is OK
- 10.5281/zenodo.1442734 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 11, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 11, 2019

@labarba Do you know how to debug the crossref issue?

@jedbrown jedbrown added the accepted label Apr 11, 2019

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Apr 11, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
100 16 0 16 0 0 200 0 --:--:-- --:--:-- --:--:-- 202
sh: 44: Syntax error: newline unexpected
Looks like we failed to compile the Crossref XML

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019


OK DOIs

- 10.1137/1.9781611970104 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.7717/peerj.453 is OK
- 10.5281/zenodo.1442734 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@labarba

This comment has been minimized.

Copy link
Member

labarba commented Apr 11, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019


OK DOIs

- 10.1137/1.9781611970104 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.7717/peerj.453 is OK
- 10.5281/zenodo.1442734 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 11, 2019

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

% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
100 16 0 16 0 0 486 0 --:--:-- --:--:-- --:--:-- 500
sh: 44: Syntax error: newline unexpected
Looks like we failed to compile the Crossref XML

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Apr 11, 2019

I have no clue what is going on — @arfon, do you?

@arfon

This comment has been minimized.

Copy link
Member

arfon commented Apr 12, 2019

Looks like Whedon doesn't like apostrophes in names (this feels like a new bug as I don't think I've seen it before). Short term fix here: PyWavelets/pywt#487

@grlee77

This comment has been minimized.

Copy link
Collaborator

grlee77 commented Apr 12, 2019

Thanks @arfon. I merged your PR. Can you check if Whedon is happy now?

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 12, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 12, 2019


OK DOIs

- 10.1137/1.9781611970104 is OK
- 10.1109/MCSE.2010.118 is OK
- 10.7717/peerj.453 is OK
- 10.5281/zenodo.1442734 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 12, 2019

Check final proof 👉 openjournals/joss-papers#619

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

@whedon accept deposit=true
@labarba

This comment has been minimized.

Copy link
Member

labarba commented Apr 12, 2019

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 12, 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#620
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01237
  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...

@labarba

This comment has been minimized.

Copy link
Member

labarba commented Apr 12, 2019

Congratulations, @grlee77, your JOSS paper is published! 🚀

Big thanks to our editor: @jedbrown, and reviewers: @rafat, @souopgui — your contribution is valued by all of us! 🙏

@labarba labarba closed this Apr 12, 2019

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 12, 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](http://joss.theoj.org/papers/10.21105/joss.01237/status.svg)](https://doi.org/10.21105/joss.01237)

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

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

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:

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