Joss-reviews: [REVIEW]: WEdiff: A Python and C++ package for automatic differentiation

[whedon]

Submitting author: @ctdegroot (Christopher DeGroot)
Repository: https://bitbucket.org/cdegroot/wediff
Version: N/A
Editor: @arfon
Reviewer: @highlando, @kyleniemeyer
Archive: 10.5281/zenodo.1495614

Status

Status badge code:

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

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

@highlando & @kyleniemeyer, 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.

Review checklist for @highlando

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 (N/A)?
• [x] Authorship: Has the submitting author (@ctdegroot) 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)?

Review checklist for @kyleniemeyer

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 (N/A)?
• [x] Authorship: Has the submitting author (@ctdegroot) 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]

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

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

1. 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 :page_facing_up: <--

[arfon]

@highlando, @kyleniemeyer - please try and carry out your reviews soon please 🙏

[highlando]

I'm done with my review... Didn't check 3 boxes. Which, however, I don't find critical.

• [x] guideline for contributions/support -- there is none but the repo is hosted on bitbucket and open for issues (and probably pull requests)
• [ ] statement of need -- not in the documentation but in the JOSS paper
• [x] no DOIs in the JOSS paper references -- this might be fixed easily

I had a few minor issues and wishes for improvements, which were resolved immediately:

1. example for python
2. installation without root privileges
3. location of the example file

[ctdegroot]

Thanks @highlando for your review!

I've added a few guidelines to the repo regarding contributions/support now. I've also added DOIs to the references that have them. Let me know if I should add the statement of need also to the documentation or if it is ok in the JOSS paper.

[ctdegroot]

@whedon commands

[whedon]

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

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

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

[ctdegroot]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

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

[highlando]

@ctdegroot @arfon I'm perfectly fine with everything!!!

[arfon]

@kyleniemeyer - when do you think you might be able to complete your review by?

[ctdegroot]

@arfon - since we haven’t heard anything from @kyleniemeyer in a couple of months, I’m wondering what the next steps in the process are?

[kyleniemeyer]

@arfon @ctdegroot sorry for my lack of response. I will complete my review by the end of the week.

[ctdegroot]

@kyleniemeyer - Great! Thanks for your quick reply!

[arfon]

:wave: @kyleniemeyer - friendly reminder to complete this review when you get a chance please :-)

[kyleniemeyer]

Some comments (will add more as I go)

Installation info

The installation info is pretty specific to Linux machines. Also, perhaps installation would be made easier by creating a conda package for the software? Right now installation is all manual, including dependencies, and something like this would make it a lot easier.

I did run into a problem when building the software, which I submitted as an issue: https://bitbucket.org/cdegroot/wediff/issues/4/build-error-static-vs-shared

[ctdegroot]

Thanks for the comments so far. I would like to be able to create a conda package, but I have not yet figured out a way to do so, given that it also relies on compiled C++ libraries. I will continue to look into this because I agree that the installation procedure could be improved, if that is possible.

[kyleniemeyer]

@ctdegroot conda is not limited to Python packages—you can definitely include C++ libraries. For example, the Cantera package also uses scons to build from source, and requires boost (plus a few other libraries): https://github.com/Cantera/conda-recipes/tree/master/cantera

[ctdegroot]

@kyleniemeyer Thanks for the link; it looks helpful. I'll need to spend some time to implement that for my code.

[kyleniemeyer]

@ctdegroot just a suggestion, but it would definitely help.

[ctdegroot]

@kyleniemeyer I agree with the suggestion. I've been looking to do something like this anyways.

[kyleniemeyer]

For the paper, I've submitted a few minor fixes myself (https://bitbucket.org/cdegroot/wediff/pull-requests/1/), and have some additional comments:

• (putting on my JOSS editor hat for a minute) Could you add more-complete affiliation info (e.g., location)?
• At the beginning of summary, it would be useful to have some more precise/specific examples for the need, rather than just "development of all types of codes that do numerical computations".
• In the equation, should the LHS term be \frac{\partial f_n}{\partial f_0} ? Or else should the first term on the RHS be \frac{\partial f}{\partial f_{n-1}}?
• Your second paragraph in Summary implicitly references the software package, but it isn't explicitly referenced until the third paragraph

[ctdegroot]

Thanks; I've accepted your changes in the pull request. I'll look into these additional items as soon as I can. I'm making some progress on the conda pacakge as well, but have no idea what is going on with the swig error at this point in time (we have the same platform and versions of everything and mine works fine).

[kyleniemeyer]

@ctdegroot After playing around with it a bit more, and installing swig using homebrew rather than miniconda, I was able to get it to build!

[ctdegroot]

@kyleniemeyer Great! Sorry it wasn't very convenient to build. Hopefully the conda package will be a lot easier!

[kyleniemeyer]

So while the scons build went fine, I'm trying to follow your instructions about running the Python unit tests (pytest build). I have not done scons install, because (1) that comes after this step in the README and (2) I'd prefer not to install something before running the tests. However, I get this error from pytest:

build/test_assignment.py:17: in <module>
    from wediff.double import FwdDiff
E   ModuleNotFoundError: No module named 'wediff'

and so on from the other test files.

Is it required to install the package before the Python tests can be run? I would have thought that it should be possible to run the test suite on the built module before installing it.

[kyleniemeyer]

Update: scons install, followed by export PYTHONPATH=/usr/local/lib/WEdiff/ (conda/Anaconda doesn't use PYTHONPATH, but you can add things there that aren't installed with conda), allowed me to run the Python tests.

[ctdegroot]

As it is currently set up it should be installed before running the tests. It would be possible to run from the build directory actually if you added that to the PYTHONPATH instead of the one listed above.

[kyleniemeyer]

Question about the example: the README makes it sound like there are multiple examples, but I only see the one example. Are there supposed to be more? (I'm not saying there need to be, in part because you give multiple examples in the ipython notebook, just a clarification).

[kyleniemeyer]

OK, aside from the open issues above, everything else looks good.

If you are currently working on a conda package, I'll hold off on the installation checkbox above, because I do think that would really simplify installation and handle the dependencies.

[ctdegroot]

@kyleniemeyer There is only one example in the README; the rest are in the notebook. I can clarify this. I am working on the conda package. Give me a day or two and I'll report back on that.

[kyleniemeyer]

@ctdegroot sounds good, thanks!

[ctdegroot]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

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

[ctdegroot]

@kyleniemeyer I've made all of the requested changes to the paper and the README. I haven't had much of a chance yet to work on the conda package yet, but it will be coming soon.

[ctdegroot]

@kyleniemeyer Ok, I finally got the conda recipe working. It took a really long time to figure out, but there was an issue with conda doing static linking with the Python library and swig doing dynamic linking. Long story short, I finally got rid of a pesky segfault.

The package can be found here: https://anaconda.org/cdegroo5/wediff

Right now I've just done the MacOS version, but the Linux one should be straightforward. I'm not planning to look into Windows support for the time being. Can you check that it works for you?

In order to get the conda recipe working, I had to change the install procedure somewhat, but it made it more robust. I will be updating the install instructions in the README shortly.

Can you let me know if there are any more issues that need to be addressed before closing out this review?

[kyleniemeyer]

Hi @ctdegroot, I could install the conda package fine, but ran into some problems when trying to import within python:

Traceback (most recent call last):
  File "/usr/local/miniconda3/envs/wediff/lib/python3.7/site-packages/wediff/fwd_diff.py", line 14, in swig_import_helper
    return importlib.import_module(mname)
  File "/usr/local/miniconda3/envs/wediff/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 965, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'wediff._fwd_diff'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/miniconda3/envs/wediff/lib/python3.7/site-packages/wediff/__init__.py", line 15, in <module>
    from wediff.fwd_diff import FwdDiffManager
  File "/usr/local/miniconda3/envs/wediff/lib/python3.7/site-packages/wediff/fwd_diff.py", line 17, in <module>
    _fwd_diff = swig_import_helper()
  File "/usr/local/miniconda3/envs/wediff/lib/python3.7/site-packages/wediff/fwd_diff.py", line 16, in swig_import_helper
    return importlib.import_module('_fwd_diff')
  File "/usr/local/miniconda3/envs/wediff/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
ModuleNotFoundError: No module named '_fwd_diff'

(To replicate, I created a new conda environment called wediff with conda create -n wediff, then did source activate wediff and conda install -c cdegroo5 wediff.)

[ctdegroot]

@kyleniemeyer That was my mistake. It was picking up the _fwd_diff file from another installation, not the conda one. I have uploaded the package again and I think it should work. Works on my system with the other installation removed.

[kyleniemeyer]

@ctdegroot I'm sorry, but I just tried again and got the same error as above, after removing the wediff install and then reinstalling it (and confirming that no other package is present, at least via python). Could it still be picking up the other installed package on your system?

[ctdegroot]

@kyleniemeyer Just checked by importing wediff and issuing command "wediff._fwd_diff.__file__", which confirms that it is the one installed by conda. So, it's working on my system.

[ctdegroot]

@kyleniemeyer Can you check if this file exists on your system? /usr/local/miniconda3/envs/wediff/lib/python3.7/site-packages/wediff/_fwd_diff.so

[kyleniemeyer]

@ctdegroot the contents of /usr/local/miniconda3/envs/wediff/lib/python3.7/site-packages/wediff are __init__.py __pycache__ double fwd_diff.py test.

[ctdegroot]

@kyleniemeyer I've just uninstalled from my system and verified that the file in question is removed. After reinstalling it is back and the package works. Could you perhaps try installing again? Maybe somehow the downloaded package was cached from the previous version? I don't know all of the details of how conda packages work, but it seems like you are getting the old version somehow.

[ctdegroot]

@kyleniemeyer If you download the tar.bz2 file from https://anaconda.org/cdegroo5/wediff/files you can see that the _fwd_diff.so file should be in the site-packages directory.

[kyleniemeyer]

@ctdegroot got it! I had to clean conda's cache, after which reinstalling the package worked fine.

(I imagine this is because the fixed package had the same version number, so conda didn't think it needed to download anything. )

Last change needed: you should add a mention of the conda package as another way to install wediff in the README. With that, I'm happy to recommend acceptance.

[ctdegroot]

@kyleniemeyer Ok, great! I've updated the README and tagged a version v1.0 to match with the paper. Thanks for all of your feedback here!

[kyleniemeyer]

@arfon I recommend accepting now!

[arfon]

@ctdegroot - 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.

[ctdegroot]

@arfon Done. http://doi.org/10.5281/zenodo.1495614

[arfon]

@whedon set 10.5281/zenodo.1495614 as archive

[whedon]

OK. 10.5281/zenodo.1495614 is the archive.

[arfon]

@whedon accept

[whedon]

Attempting dry run of processing paper acceptance...

[whedon]

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/78

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/78, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.
@whedon accept deposit=true

[arfon]

@whedon accept deposit=true

[whedon]

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

[whedon]

🚨🚨🚨 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 :point_right: https://github.com/openjournals/joss-papers/pull/79
2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.00820
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...

[arfon]

@highlando, @kyleniemeyer - many thanks for your reviews here ✨

@ctdegroot - your paper is now accepted into JOSS! :zap::rocket::boom:

[whedon]

: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 snippets:

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

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

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

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=