[REVIEW]: fgivenx: a python package for functional posterior plotting

[whedon]

Submitting author: @williamjameshandley (Will Handley)
Repository: https://github.com/williamjameshandley/fgivenx
Version: v2.1.0
Editor: @arfon
Reviewer: @FaustinCarter, @zhampel
Archive: 10.5281/zenodo.1404584

Status

Status badge code:

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

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

@FaustinCarter & @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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arfon know.

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

Review checklist for @FaustinCarter

Conflict of interest

• 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

• I confirm that I read and will adhere to the JOSS 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 (v2.1.0)?
• Authorship: Has the submitting author (@williamjameshandley) 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 @zhampel

Conflict of interest

• 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

• I confirm that I read and will adhere to the JOSS 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 (v2.1.0)?
• Authorship: Has the submitting author (@williamjameshandley) 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]

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

2. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

For a list of things I can do to help you, just type:

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[FaustinCarter]

I have some specific comments on the paper which I outline here by paragraph. I'll have comments on the code soon.

Paragraph 1:

• Why does this end in a colon? Are you about to present an example?
• Your tense is off. I think you mean "probability distributions" rather than "a probability distribution" (which would imply a single distribution for all unknown parameters in the universe!)

Paragraph 2:

• I'm not sure what the point of this paragraph is. The age of the universe and the idea that space expands don't seem to be relevant at any further point in the paper.

Paragraph 3:

• Plots like what? Are you suggesting that the idea in the above paragraph could be parameterized into two variables that have a joint probability distribution? What are those variables? Can you include the plot of those variables made from your software?

Paragraph 4:

• scipy, getdist, and corner all have instructions on how to cite them in an academic publication. It would be good to include those citations in addition to links to their webpages. A reference to a MCMC paper would also be good to make it easy for readers to look that up.
• I'll also put in a shameless plug here for the pygtc package (10.21105/joss.00046) which is an alternative to corner and probably belongs in that list, although that's up to you.
• After a more careful reading I think you are implying that corner plots are nice, but there is more to the story (i.e. looking at the actual model described by all the MCMC realizations?) and that's where your software comes in? I think you need to make this more explicit.

Paragraph 5:

• Mixed voice. You switch between third person (one does, one could) and first person plural (we do, we could, our ability). It would be good to unify the voice over the whole paper.
• Is the phrase "plotted thus" supposed to be a reference to Fig. 1? If so, say something like: "in Fig. 1 panel (a) we plot the parameter covariances between m and c for X realizations (blue) and Y realizations (red). In panel (b) we plot the function y=mx+c for X realizations. In panel (c) we plot something called DxL which is never mentioned in the paper and should be. In panel (d) we plot the probability of measuring y for a given x, which is essential a contour version of panel (b)."
• Why is there a red distribution and a blue distribution? Is that two separate sets of fake data that are being fit to the same model? Edit: Now that I read your example code more carefully I see that the blue points are the priors. This is not clear from the paper (and should be).

Paragraph 6:

• A couple references seem broken.

Paragraph 7:

• The first line of this is the first mention you make of what your package actually does in words. I think this should come earlier and there should be a little more description of what fgivenx is designed to do right from the start.

[FaustinCarter]

A few comments on the install process:

• I could not find installation instructions anywhere ( so I just guessed with pip). I also could not find a link to the documentation? Is it hosted somewhere, or is the user supposed to build it themselves? If the second, it would be good to include installation instructions and document building instructions directly in the github readme and in the description on PyPI.

• No description of anything at all on the PyPI page for fgivenx.

• For some reason the tests directory is not included in the source that is available at PyPI (this is v. 2.1.0), which means the tests do not install with pip. They are available in the zipped up source files in the github release.

• The tests passed except for one which failed when it tried to import getdist. I think that since getdist is not a dependency of fgivenx, the tests should not fail if it is not installed. Instead, the code should check to see if getdist is available and skip the test in question if the import fails.

[williamjameshandley]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[williamjameshandley]

@FaustinCarter

My apologies for the poor initial state of the paper, I'm new to this system, and it wasn't clear to me initially how to preview the paper rendering prior to submission.

Re paras 2 and 3, there was a missing figure, the presence of which hopefully makes them more clear.

I believe I have addressed all of the issues you raised, but it wasn't clear to me how to cite getdist

[williamjameshandley]

@FaustinCarter

• I could not find installation instructions anywhere ( so I just guessed with pip). I also could not find a link to the documentation? Is it hosted somewhere, or is the user supposed to build it themselves? If the second, it would be good to include installation instructions and document building instructions directly in the github readme and in the description on PyPI.
• No description of anything at all on the PyPI page for fgivenx.
• For some reason the tests directory is not included in the source that is available at PyPI (this is v. 2.1.0), which means the tests do not install with pip. They are available in the zipped up source files in the github release.

I've updated the README.md, and now updated the pypi so that it mirrors it, you can see it in alpha state here. Tests now included in pypi.

The tests passed except for one which failed when it tried to import getdist. I think that since getdist is not a dependency of fgivenx, the tests should not fail if it is not installed. Instead, the code should check to see if getdist is available and skip the test in question if the import fails.

I've now done this, but I'm not sure in the best way to implement this (In particular, it broke my 100% code coverage).

[williamjameshandley]

I've now updated both pip and readthedocs so that they mirror the README.rst file in the GitHub repo, and added the additional information that was requested to the README. I also expanded the test suite so that it tests with and without the optional dependencies

[williamjameshandley]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[FaustinCarter]

Round 2

OK, I have some new comments! I actually tried it out on some data and made a pretty sweet plot of some residuals. I had to wrap my function to get the order of the args correct for your code (and to make it plot residuals rather than the function), but it worked just fine.

I also reinstalled the latest version in a fresh environment and all the tests ran just fine (although installing matplolib via pip in a conda environment on OSX breaks everything...). OK, now onto comments. Once these are addressed I can check off the last few boxes above and recommend you for publication.

General

• The version on PyPI is 2.1.8. The latest release on github is 2.0.0, which means the Zenodo DOI is for v. 2.0.0. I think if you generate a new release of 2.1.8, then the Zenodo hook should automatically update you with a new DOI for 2.1.8. Then I can check the version box on the checklist above.

• Citing getdist: I ran into the same problem when I was trying to cite them. I eventually settled with the following citation: Lewis, Antony. 2015. “Getdist Github Repository.” https://github.com/cmbant/getdist

• I think that the file outlining contributions could be added to the end of the readme. That's a more natural place for it, and it encourages people to get involved right from the start!

• In general I strongly feel that __init__.py should be reserved for initializing things. This is perhaps my interpretation of what being Pythonic should mean, but in my view a better (and more consistent) approach would be to put your top-level functions in a file called fgivenx.py, and then use the __init__.py file to import those into the package namespace (from .fgivenx import *, or just defining them in __all__). Now you have access to those from the top level, but your users can easily locate them in your package (instead of hunting around for ages until finally thinking to look in the __init__.py file). The Hitchhiker's Guide to Python agrees on this point (https://docs.python-guide.org/writing/structure/), but if you feel strongly about keeping it the way it is, it's your package! I'm not going to make that a requirement of the review.

• You might consider also generating a conda-forge recipe for your package in the future. Makes it easy for conda users to manage the dependencies. This is just a weak suggestion for an eventual future and doesn't affect this review.

Documentation

• In the introduction of the docs, you say the driving routine is compute_contours, but your example does not explicitly call that function. In fact, I'm not sure that funciton exists? Do you mean compute_pmf?

• The docstring for fgivenx.samples.compute_samples refers to the docstring for compute_contours. This shoudl be fixed to reference the correct function.

• The "Introduction" should be the first thing one sees upon going to the docs website, and then there can be a link to the API. Especially since the figure should directly follow the example code in the introduction, and now it is on the landing page, but doesn't show up after the example code in the introduction.

• It would be good to start a changelog and include that in the documentation somewhere.

• The docstring for compute_samples does not mention parameter logZ. Also does not mention that if logZ (apparently an array) is passed, then the user must pass a list of functions to f instead of a single function.

• The docstring of compute_pmf could be more explicit that the return value is a tuple. I had to actually go look at the code to figure out what it was really returning.

• I suggest a comprehensive review of all the docstrings to make sure they accurately reflect the function signature (and return value) and describe all the parameters, args, and kwargs. Also, each parameter should be defined in the doc string rather than saying "go look at this other docstring". Referring to another docstring is fine for kwargs that will be passed through to some other function, but not for required parameters.

Plotting

• For plotting routines, ax could be an optional argument. Then, if not supplied, it creates an axis, plots on it, and returns the axis (or maybe figure) back again. This is more of a suggestion than anything, but it makes the plotting a bit more friendly for matplotlib novices because then fgivenx.plot.plot just gives you a nice figure. I'm not going to hold the review up on this point.

• It's not clear to me what the plots test is actually doing. Ideally it would generate an image and compare it to some pre-rendered image, but I don't see you loading pytest-mpl at all. In fact, the second test doesn't even use pytest at all I don't think.

Paper

This is much, much better now. Just a few additional comments.

• You can replace "we" with "I" everywhere since you are the only author and you aren't the queen.
• Paragraph 3: should be "... a probability distribution..."
• Paragraph 3: ref to Fig. 2 should include reference to the appropriate panel.

Random

I'm mystified by line 37 in samples.py. It looks like you are zipping two lists of different length together for the case where logZ is omitted. That should just return an iterator that is the length of the shortest list (which I think is length 1), but somehow it doesn't? I see where you set f=[f], but not where you make that new list the same length as the samples dimension you are zipping against. How does this even work?

[williamjameshandley]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[williamjameshandley]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[williamjameshandley]

Nice plots!

[williamjameshandley]

General

• The version on PyPI is 2.1.8. The latest release on github is 2.0.0, which means the Zenodo DOI is for v. 2.0.0. I think if you generate a new release of 2.1.8, then the Zenodo hook should automatically update you with a new DOI for 2.1.8. Then I can check the version box on the checklist above.

I've updated the new version to 2.1.9, and created a release so that Zenodo is up-to-date.

• Citing getdist: I ran into the same problem when I was trying to cite them. I eventually settled with the following citation: Lewis, Antony. 2015. “Getdist Github Repository.” https://github.com/cmbant/getdist
• I think that the file outlining contributions could be added to the end of the readme. That's a more natural place for it, and it encourages people to get involved right from the start!

Both of these are now done.

• In general I strongly feel that__init__.py should be reserved for initializing things...

I've done this, by moving the relevant code to fgivenx/drivers.py and importing it in __init__.py. I couldn't get fgivenx/fgivenx.py to work for python2 in a clean manner.

• You might consider also generating a conda-forge recipe for your package in the future. Makes it easy for conda users to manage the dependencies. This is just a weak suggestion for an eventual future and doesn't affect this review.

I don't really have any experience of using conda, but know that a lot of people do. One way to make sure that this does get done in the future would be to post an 'issue' to the GitHub -- that way I'll be reminded/encouraged to get round to this at some point in the future.

[williamjameshandley]

Documentation

• In the introduction of the docs, you say the driving routine is compute_contours, but your example does not explicitly call that function. In fact, I'm not sure that funciton exists? Do you mean compute_pmf?
• The docstring for fgivenx.samples.compute_samples refers to the docstring for compute_contours. This shoudl be fixed to reference the correct function.

I've now introduced three new driver functions fgivenx.plot_contours, fgivenx.plot_lines, and fgivenx.plot_dkl, and changed all references to those

• The "Introduction" should be the first thing one sees upon going to the docs website, and then there can be a link to the API. Especially since the figure should directly follow the example code in the introduction, and now it is on the landing page, but doesn't show up after the example code in the introduction.
• It would be good to start a changelog and include that in the documentation somewhere.

Done

• The docstring for compute_samples does not mention parameter logZ. Also does not mention that if logZ (apparently an array) is passed, then the user must pass a list of functions to f instead of a single function.

I think I've clarified this.

• The docstring of compute_pmf could be more explicit that the return value is a tuple. I had to actually go look at the code to figure out what it was really returning.

Currently the return section of the docstring has a list of the two arguments. What do you think would make this clearer?

• I suggest a comprehensive review of all the docstrings to make sure they accurately reflect the function signature (and return value) and describe all the parameters, args, and kwargs. Also, each parameter should be defined in the doc string rather than saying "go look at this other docstring". Referring to another docstring is fine for kwargs that will be passed through to some other function, but not for required parameters.

Done.

[williamjameshandley]

Plotting

• For plotting routines, ax could be an optional argument. Then, if not supplied, it creates an axis, plots on it, and returns the axis (or maybe figure) back again. This is more of a suggestion than anything, but it makes the plotting a bit more friendly for matplotlib novices because then fgivenx.plot.plot just gives you a nice figure. I'm not going to hold the review up on this point.

I think this a great idea. I've leveraged plt.gca() to get this behaviour. If ax is not passed, then it just plots on the last axis, or creates one if there isn't one, consistent with the way that matplotlib.pyplot.plot works.

• It's not clear to me what the plots test is actually doing. Ideally it would generate an image and compare it to some pre-rendered image, but I don't see you loading pytest-mpl at all. In fact, the second test doesn't even use pytest at all I don't think.

I've added a matplotlib test that checks that it produces the correct image to this test.

[williamjameshandley]

Random

• I'm mystified by line 37 in samples.py. It looks like you are zipping two lists of different length together for the case where logZ is omitted. That should just return an iterator that is the length of the shortest list (which I think is length 1), but somehow it doesn't? I see where you set f=[f], but not where you make that new list the same length as the samples dimension you are zipping against. How does this even work?

This function assumes that it's given a list of functions and a list of samples, i.e. it's been already pre-processed to be evidence-weighted as in fgivenx.drivers.compute_pmf. I've updated the docstring to reflect this.

[williamjameshandley]

Many thanks for all these comments. I think this is really good process for improving distributed code.

[zhampel]

At the moment I have only one comment in my notes regarding the code:

• I really like the kwargs.pop functionality you put in. So smooth :)

For now, it's really just ensuring the docstrings are uber clear, and I'll finish up my code review.

[williamjameshandley]

@zhampel Many thanks for your detailed comments.

• Paper: I think I've addressed all your issues.
• Documentation: I've tried to make the docstrings as descriptive as possible, without being too verbose. I agree the evidence was unclear. Hopefully now it's relatively obvious what these functions are expecting as an optional argument.
• Installation. Updated the readme accordingly. Does installing Pillow fix your issues? If so I'll update the Readme for mac-users.

Once you're happy with my updates (all on github), and we've fixed the OSX failure, I'll update the version numbers and PyPi.

I really like the kwargs.pop functionality you put in. So smooth :)

I don't know how to do it any other way now. It came from Effective Python, which I thoroughly recommend (and is split up into nice, bitesize examples).

[zhampel]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[zhampel]

@williamjameshandley

• For the paper edit, you are missing periods in the last sentence of the last paragraph, as well as one at the end of the caption of Figure 1.
• From your reply to the the documentation comments, it seemed like you were going to make edits to the docstrings. I didn't see any commits to that end. Were you planning on doing so or just agreeing that there was some ambiguity there?
• I will try the Pillow install today to check your fix.

[zhampel]

@williamjameshandley

• I must not have refreshed my page properly or something, so I finally saw the docstring edits you made.
• After installing pillow now, I get some ignored QueueManagerThread warning, but no error.

Good job cleaning this up!

[zhampel]

@williamjameshandley

Code

A few comments about the code. They are minor and nit-picky. Everything else looks well-written and functionally appropriate.

• _check_args in _utils.py: There are a few extra indents on some raise ValueError statements
• _normalize_weights in _utils.py: Could use a little more space for readability. It's a little tight compared to the rest of your code.

Maybe rerun flake8 just to check one last time to make sure there aren't any other stragglers.

One more thing about the testing suite:

• is test_plot_lines actually running a test? I don’t see an image_comparison style test like you have in test_plotting in test_drivers.py.

[zhampel]

@arfon I've checked off all my boxes and consider my review finished. @williamjameshandley needs to add a couple periods to the paper and I have made a few last minor comments concerning the code, though they are stylistic and do not require my approval for publication.

[arfon]

⚡ thanks @zhampel.

@williamjameshandley - please let me know when you've had a chance to respond to @zhampel and @@FaustinCarter's feedback.

[williamjameshandley]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[williamjameshandley]

@zhampel

• I must not have refreshed my page properly or something, so I finally saw the docstring edits you made.

I find one often needs to force refresh the cache in order to see updates

Maybe rerun flake8 just to check one last time to make sure there aren't any other stragglers.

Done

is test_plot_lines actually running a test? I don’t see an image_comparison style test like you have in test_plotting in test_drivers.py.

I've added some more tests to this, so that each separate test actually runs image_comparison tests. Tests and figures also uploaded to PyPi.

After installing pillow now, I get some ignored QueueManagerThread warning, but no error.

Added a note to the README.rst

@arfon I believe that I have now addressed all the issues raised

[williamjameshandley]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof 📄 <--

[arfon]

@williamjameshandley - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

[williamjameshandley]

@arfon Zenodo Link

[arfon]

@whedon set 10.5281/zenodo.1404584 as archive

[whedon]

OK. 10.5281/zenodo.1404584 is the archive.

[arfon]

@FaustinCarter, @zhampel - many thanks for your reviews here ✨

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

[whedon]

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

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

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=