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]: PyUnfold: A Python Package for Iterative Unfolding #741

Closed
whedon opened this Issue May 18, 2018 · 55 comments

Comments

Projects
None yet
7 participants
@whedon
Collaborator

whedon commented May 18, 2018

Submitting author: @zhampel (Zigfried Hampel-arias)
Repository: https://github.com/jrbourbeau/pyunfold
Version: v0.4
Editor: @lheagy
Reviewer: @juliohm, @benjaminrose
Archive: 10.5281/zenodo.1258211

Status

status

Status badge code:

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

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

@juliohm & @benjaminrose, 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 @lheagy know.

Review checklist for @juliohm

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 (v0.3)?
  • Authorship: Has the submitting author (@zhampel) 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 @benjaminrose

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 (v0.3)?
  • Authorship: Has the submitting author (@zhampel) 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.

Collaborator

whedon commented May 18, 2018

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @juliohm, 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.

Collaborator

whedon commented May 18, 2018

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

This comment has been minimized.

Collaborator

whedon commented May 18, 2018

@lheagy

This comment has been minimized.

Member

lheagy commented May 18, 2018

Many thanks @juliohm and @benjaminrose for being willing to review! In the main issue thread, there is a checklist for each of you as you go through the review. Please let me know if you have any questions or if there is anything I can assist with!

@juliohm

This comment has been minimized.

Collaborator

juliohm commented May 27, 2018

Very good documentation @zhampel, I was able to reproduce all the examples, and follow the mathematical derivations.

A few comments to improve the project further:

Conceptual

  • It would be nice to compare pyunfold with other related packages for inverse problems in Python, or clarify in the documentation the unique features of the project.
  • Additionally, it would be interesting to see examples where the observed distribution is very different than the true distribution. Or alternatively, you could list the distributional assumptions under which the method is successful. For example, can the method be applied to update a bimodal observation into an unimodal truth?

Implementation

  • Priors (or distributions) in the project are represented with a vector of numbers (densities). In the section "custom priors" you could clarify that any vector of numbers (integrating to one) can be used as a prior and that priors uniform and jeffreys are not specialized classes.
  • The binder option to run the examples online is missing the pyunfold module as reported in jrbourbeau/pyunfold#67
  • Since you are using Travis CI for the tests, you could also consider activating the build on Mac OS. You can do it by adding the section below:
os:
  - linux
  - osx
@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 27, 2018

The binder option to run the examples online is missing the pyunfold module

Thanks @juliohm for pointing this out and submitting a corresponding issue! I've updated the environment that is built to run the example notebooks on binder. pyunfold is now installed and the notebooks all work as expected.

ref: jrbourbeau/pyunfold#68

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 27, 2018

Since you are using Travis CI for the tests, you could also consider activating the build on Mac OS.

That's a good point. I added Mac OS tests for Python 3.6 in jrbourbeau/pyunfold#69.

I didn't want to double the number of jobs that are run on Travis by running both linux and osx tests for several different versions of Python. So instead I'm keeping the original jobs that run Python 2.7, 3.4, 3.5, and 3.6 on linux and have added a job for Python 3.6 on osx. This should catch if there is a Python-specific problem or a Mac OS-specific problem.

@juliohm

This comment has been minimized.

Collaborator

juliohm commented May 27, 2018

Thank you @jrbourbeau for incorporating the changes. Is there a good reason to test on all 3.x versions of Python? Are they very different?

I will convert my previous comment into a set of tasks, and will mark the tasks you've already implemented as done. ✔️

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 27, 2018

Is there a good reason to test on all 3.x versions of Python? Are they very different?

All of the existing code in PyUnfold should work just fine on all Python 3.x versions. Running the automated tests on different 3.x versions is more a preventative measure to make sure new contributions don't unexpectedly add something that isn't fully backwards compatible (e.g. Python 3.6 added a new f-string formatting syntax which are really nice, but won't work on Python 3.4 or 3.5).

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 28, 2018

you could clarify that any vector of numbers (integrating to one) can be used as a prior and that priors uniform and jeffreys are not specialized classes

@juliohm I've updated the Custom Priors section in the documentation in jrbourbeau/pyunfold#70 to address the points you brought up. Hopefully it is more clear now that any given prior array can be used, not just the built-in uniform_prior or jeffreys_prior. Please let me know if you think it needs any further clarification.

Thank you for the thorough read through and feedback!

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 29, 2018

Just added the NSF to the acknowledgements section of paper.md

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 29, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Collaborator

whedon commented May 29, 2018

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

This comment has been minimized.

Collaborator

whedon commented May 29, 2018

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 29, 2018

clarify in the documentation the unique features of the project

@juliohm we've added a "Why PyUnfold?" page to the documentation that outlines the unique features of the project.

ref: jrbourbeau/pyunfold#71

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 30, 2018

Additionally, it would be interesting to see examples where the observed distribution is very different than the true distribution...For example, can the method be applied to update a bimodal observation into an unimodal truth?

@juliohm Indeed a bimodal observed distribution can be unfolded into a unimodal truth distribution. For example, in a similar fashion to the "Tutorial" notebook, one can have true, observed, and unfolded distributions as shown below.

image

I can add this as an additional example notebook to the documentation. My only concern is that it wouldn't really walk through any new functionality in PyUnfold, just simply using a different example dataset.

@juliohm

This comment has been minimized.

Collaborator

juliohm commented May 30, 2018

That is a very nice example, I think it tells a lot more about the generality of the method than the current example in the documentation, but it is up to you to decide what example is more telling for the audience you are targeting. =)

From a user's perspective, or maybe putting myself in the mind of someone that is probing different packages for inverse problems, I'd say that just knowing that the method doesn't assume gaussianity or bell shapes is a big plus. 👍

I am marking all the items as solved. Congratulations again on the package, good stuff!

@benjaminrose do you have further corrections or improvements that you would like to share?

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 30, 2018

just knowing that the method doesn't assume gaussianity or bell shapes is a big plus

That's a really good point. I'll discuss with @zhampel as to which example dataset would be more illuminating for the unfolding community.

@jrbourbeau

This comment has been minimized.

jrbourbeau commented May 30, 2018

@juliohm Thanks so much for your detailed review of PyUnfold and useful review comments! I definitely think they helped improve the project

@zhampel

This comment has been minimized.

Collaborator

zhampel commented May 31, 2018

@juliohm we've used the method successfully on power law spectra unfoldings for publications, so @jrbourbeau and I can cook up such an example in addition to our canonical examples. We just have to make sure we don't violate any public release agreements from our respective collaborations ;)

@zhampel

This comment has been minimized.

Collaborator

zhampel commented May 31, 2018

Testing: @whedon Who's your favorite Buffy character? I was always partial to Spike.

@benjaminrose

This comment has been minimized.

Collaborator

benjaminrose commented May 31, 2018

This is a great project. @juliohm's thank you for your great comments. A have a few of my own to add.

Docs - Tutorial

  • Binder is a great tool for this! Thanks for using it. Also, maybe add a line about needing to install matplotlib if you want to follow this tutorial on your own.
  • Can you point out the cause for "We can see how the structure of the 2-d response histogram differs from the normalized response matrix."
  • In the tutorial, can you either add a short explanation of the ts parameter is or just use the default values for input 20.

Docs - API

  • Can you define the ts stats in the API docs, only their acronyms are used.
    • The paper does define them, but not the API docs
  • Can you explain/link to a definition of Jeffrey's priors in the API docs? Maybe as a References header?

Docs - Contributing page

  • Thanks for explaining git fetch upstream. This is very useful for first-time contributors.

Paper

  • The statement of need is better in the docs than in the paper. I really like this section.

General

  • What is your plan to get to v1.0?
  • Walking through one simple but complete example is enough. I support the idea of at least pointing out more complicated solutions are solve-able, like the bimodal observed distribution coming from a unimodal truth. Putting it with the other advanced techniques makes sense to me.
@benjaminrose

This comment has been minimized.

Collaborator

benjaminrose commented Jun 1, 2018

The other changes look great @jrbourbeau. But I missed the API docs for the teststat class. The abbreviations that are the not fully that I was talking about were in describing the ts parameter for iterative_unfold(). I would just add a "more info" link to https://jrbourbeau.github.io/pyunfold/api.html#test-statistics at the end of that parameter's description.

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

I was talking about were in describing the ts parameter for iterative_unfold()

Gotcha, I misunderstood your comment. Thanks for noticing that! I'll update the iterative_unfold docstring accordingly

@jrbourbeau jrbourbeau referenced this issue Jun 1, 2018

Merged

JOSS review-Updates ts parameter documentation #82

1 of 2 tasks complete
@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

@benjaminrose I updated the ts parameter description with a link to the test statistics section in the API docs

ref: jrbourbeau/pyunfold#82

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

@juliohm just as an FYI the "Performance" checkbox under the "Functionality" section for your review checklist is unchecked. If there's anything else that needs to be done on my end, just let me know. Thanks!

@juliohm

This comment has been minimized.

Collaborator

juliohm commented Jun 1, 2018

@jrbourbeau I don't recall reading in the docs statements about performance? Please let me know if they exist somewhere. The checkbox should only be marked in case the software claims states explicitly that it is "high-performance" or faster than alternatives.

@benjaminrose

This comment has been minimized.

Collaborator

benjaminrose commented Jun 1, 2018

@lheagy, I think @zhampel and @jrbourbeau have a great project here and I fully endorse their paper for publication in JOSS.

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

I think the "What is unfolding?" is your strongest statement of need...If you see a simple way to add it, and you want to, I think it would make the paper better.

@benjaminrose I agree, that's a really good point. I've added the content from the "What is unfolding?" docs to the beginning of the paper. I think it helps clarify the problem we're addressing with PyUnfold without messes with the overall flow of the paper. Thanks for the feedback!

ref: jrbourbeau/pyunfold#83

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

@whedon generate pdf

@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 1, 2018

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

This comment has been minimized.

Collaborator

whedon commented Jun 1, 2018

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

The checkbox should only be marked in case the software claims states explicitly that it is "high-performance" or faster than alternatives.

@juliohm You're correct, we don't make any statements about the performance in the docs. However, I interpreted the "If there are no claims, please check off this item" sentence to mean that since we make no performance claims, then the performance checkbox should be marked as done. I could definitely be misinterpreting that though.

@juliohm

This comment has been minimized.

Collaborator

juliohm commented Jun 1, 2018

Oh maybe it is a language issue, I am not a native speaker. When I read "Check off" I interpret it as "Do not mark the box", it is probably the opposite?

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 1, 2018

Hmm yeah I took "check off" to mean "mark the box". Perhaps @lheagy can clarify this point for us.

@lheagy

This comment has been minimized.

Member

lheagy commented Jun 3, 2018

Hi @juliohm and @jrbourbeau: sorry for the confusion (and I appreciate knowing that this is a potential point of confusion for non-native speakers). By "check off", we do mean that you can "mark the box" if there are no claims made.

@juliohm

This comment has been minimized.

Collaborator

juliohm commented Jun 3, 2018

Thank you @lheagy, checking it off 👌

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 3, 2018

Thanks for such a quick reply @juliohm!

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 3, 2018

@lheagy at this point both the reviewers have approved the paper / project for publication in JOSS. Are there any additional steps I need to take in order to move forward with publication? Thanks!

@lheagy

This comment has been minimized.

Member

lheagy commented Jun 3, 2018

Excellent! Thank you @juliohm and @benjaminrose for your review!!

@zhampel and @jrbourbeau at this point, could you please archive your software on zenodo or similar and post the doi here?

@zhampel

This comment has been minimized.

Collaborator

zhampel commented Jun 3, 2018

@lheagy: @jrbourbeau has archived on zenodo with DOI

@lheagy

This comment has been minimized.

Member

lheagy commented Jun 3, 2018

Thanks @zhampel: quick question for you - it looks like the version you archived is v0.4. This is the version we should list with the publication?

@zhampel

This comment has been minimized.

Collaborator

zhampel commented Jun 3, 2018

Yes, I think so, is that correct @jrbourbeau ? I know originally when we submitted we listed as v0.3

@jrbourbeau

This comment has been minimized.

jrbourbeau commented Jun 3, 2018

it looks like the version you archived is v0.4. This is the version we should list with the publication?

@lheagy correct, thanks for pointing that out! I'd like to have v0.4 listed with the publication as it includes updates in response to reviewer comments.

@lheagy

This comment has been minimized.

Member

lheagy commented Jun 3, 2018

👋 Hi @arfon: This submission is ready to be published!

@arfon arfon added the accepted label Jun 4, 2018

@arfon

This comment has been minimized.

Member

arfon commented Jun 4, 2018

@juliohm, @benjaminrose - many thanks for your reviews here and to @lheagy for editing this submission

@zhampel - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00741 ⚡️ 🚀 💥

@arfon arfon closed this Jun 4, 2018

@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 4, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

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

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:

@zhampel

This comment has been minimized.

Collaborator

zhampel commented Jun 4, 2018

Thank you so much @lheagy @juliohm @benjaminrose for your thorough and speedy review! 🎉 We look forward to further contributions to JOSS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment