[REVIEW]: WEdiff: A Python and C++ package for automatic differentiation #820

whedon · 2018-07-11T13:58:37Z

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:

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

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

Review checklist for @highlando

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

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

The text was updated successfully, but these errors were encountered:

whedon · 2018-07-11T13:58:40Z

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 🎉.

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

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

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 · 2018-07-11T13:58:41Z

Attempting PDF compilation. Reticulating splines etc...

whedon · 2018-07-11T13:59:10Z

--> Check article proof 📄 <--

arfon · 2018-08-19T17:57:32Z

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

highlando · 2018-09-06T12:21:40Z

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

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

ctdegroot · 2018-09-06T17:14:35Z

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 · 2018-09-06T17:15:09Z

@whedon commands

whedon · 2018-09-06T17:15:12Z

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 · 2018-09-06T17:15:29Z

@whedon generate pdf

whedon · 2018-09-06T17:15:32Z

Attempting PDF compilation. Reticulating splines etc...

whedon · 2018-09-06T17:15:58Z

👉 Check article proof 📄 👈

highlando · 2018-09-07T12:02:31Z

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

arfon · 2018-09-10T11:18:11Z

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

ctdegroot · 2018-10-09T03:34:20Z

@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 · 2018-10-09T03:53:01Z

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

ctdegroot · 2018-10-09T03:57:06Z

@kyleniemeyer - Great! Thanks for your quick reply!

arfon · 2018-11-04T18:47:12Z

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

kyleniemeyer · 2018-11-06T17:22:23Z

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 · 2018-11-06T17:38:56Z

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 · 2018-11-06T17:42:49Z

@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 · 2018-11-06T17:45:29Z

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

kyleniemeyer · 2018-11-06T17:52:16Z

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

ctdegroot · 2018-11-06T17:53:26Z

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

kyleniemeyer · 2018-11-06T19:36:44Z

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 · 2018-11-06T19:40:53Z

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 · 2018-11-06T19:45:32Z

@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 · 2018-11-06T19:46:25Z

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

ctdegroot · 2018-11-11T19:28:17Z

@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 · 2018-11-19T16:50:37Z

@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 · 2018-11-20T18:28:27Z

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 · 2018-11-20T19:18:47Z

@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 · 2018-11-20T23:56:25Z

@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 · 2018-11-21T01:00:02Z

@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 · 2018-11-21T15:05:28Z

@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 · 2018-11-21T19:59:56Z

@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 · 2018-11-21T20:51:46Z

@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 · 2018-11-21T20:54:03Z

@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 · 2018-11-21T23:06:56Z

@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 · 2018-11-22T01:28:57Z

@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 · 2018-11-24T15:57:46Z

@arfon I recommend accepting now!

arfon · 2018-11-24T21:50:57Z

@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 · 2018-11-24T22:10:20Z

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

arfon · 2018-11-25T02:50:53Z

@whedon set 10.5281/zenodo.1495614 as archive

whedon · 2018-11-25T02:50:56Z

OK. 10.5281/zenodo.1495614 is the archive.

arfon · 2018-11-25T02:51:15Z

@whedon accept

whedon · 2018-11-25T02:51:18Z

Attempting dry run of processing paper acceptance...

whedon · 2018-11-25T02:51:54Z

Check final proof 👉 openjournals/joss-papers#78

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#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 · 2018-11-25T02:52:38Z

@whedon accept deposit=true

whedon · 2018-11-25T02:52:41Z

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

whedon · 2018-11-25T02:53:17Z

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.00820 joss-papers#79
Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.00820
If everything looks good, then close this review issue.
Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

arfon · 2018-11-25T02:54:29Z

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

@ctdegroot - your paper is now accepted into JOSS! ⚡🚀💥

whedon · 2018-11-25T02:54:32Z

🎉🎉🎉 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.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=

whedon assigned arfon Jul 11, 2018

whedon added the review label Jul 11, 2018

whedon mentioned this issue Jul 11, 2018

[PRE REVIEW]: WEdiff: A Python and C++ package for automatic differentiation #794

Closed

whedon added the accepted label Nov 25, 2018

arfon closed this as completed Nov 25, 2018

whedon added published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. labels Mar 2, 2020

editorialbot mentioned this issue Nov 2, 2023

[PRE REVIEW]: Catalyst: a Python JIT compiler for auto-differentiable hybrid quantum programs #6012

Closed

[REVIEW]: WEdiff: A Python and C++ package for automatic differentiation #820

[REVIEW]: WEdiff: A Python and C++ package for automatic differentiation #820

Comments

whedon commented Jul 11, 2018 • edited Loading

Status

Reviewer instructions & questions

Review checklist for @highlando

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @kyleniemeyer

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented Jul 11, 2018

whedon commented Jul 11, 2018

whedon commented Jul 11, 2018

arfon commented Aug 19, 2018

highlando commented Sep 6, 2018 • edited Loading

ctdegroot commented Sep 6, 2018

ctdegroot commented Sep 6, 2018

whedon commented Sep 6, 2018

ctdegroot commented Sep 6, 2018

whedon commented Sep 6, 2018

whedon commented Sep 6, 2018

highlando commented Sep 7, 2018

arfon commented Sep 10, 2018

ctdegroot commented Oct 9, 2018

kyleniemeyer commented Oct 9, 2018

ctdegroot commented Oct 9, 2018

arfon commented Nov 4, 2018

kyleniemeyer commented Nov 6, 2018

Installation info

ctdegroot commented Nov 6, 2018

kyleniemeyer commented Nov 6, 2018

ctdegroot commented Nov 6, 2018

kyleniemeyer commented Nov 6, 2018

ctdegroot commented Nov 6, 2018

kyleniemeyer commented Nov 6, 2018

ctdegroot commented Nov 6, 2018

kyleniemeyer commented Nov 6, 2018

ctdegroot commented Nov 6, 2018

ctdegroot commented Nov 11, 2018

ctdegroot commented Nov 19, 2018

kyleniemeyer commented Nov 20, 2018 • edited Loading

ctdegroot commented Nov 20, 2018

kyleniemeyer commented Nov 20, 2018

ctdegroot commented Nov 21, 2018

ctdegroot commented Nov 21, 2018

kyleniemeyer commented Nov 21, 2018

ctdegroot commented Nov 21, 2018

ctdegroot commented Nov 21, 2018

kyleniemeyer commented Nov 21, 2018 • edited Loading

ctdegroot commented Nov 22, 2018

kyleniemeyer commented Nov 24, 2018

arfon commented Nov 24, 2018

ctdegroot commented Nov 24, 2018

arfon commented Nov 25, 2018

whedon commented Nov 25, 2018

arfon commented Nov 25, 2018

whedon commented Nov 25, 2018

whedon commented Nov 25, 2018

arfon commented Nov 25, 2018

whedon commented Nov 25, 2018

whedon commented Nov 25, 2018

arfon commented Nov 25, 2018

whedon commented Nov 25, 2018

whedon commented Jul 11, 2018 •

edited

Loading

highlando commented Sep 6, 2018 •

edited

Loading

kyleniemeyer commented Nov 20, 2018 •

edited

Loading

kyleniemeyer commented Nov 21, 2018 •

edited

Loading