[REVIEW]: hIPPYlib: An Extensible Software Framework for Large-Scale Inverse Problems

whedon commented 6 years ago

Submitting author: @uvilla (Umberto Villa) Repository: https://github.com/hippylib/hippylib Version: 2.1.0 Editor: @lheagy Reviewer: @IgorBaratta, @boehmc Archive: 10.5281/zenodo.596931

Status

Status badge code:

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

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

@IgorBaratta & @boehmc, 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 @lheagy know.

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

Review checklist for @IgorBaratta

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 (2.1.0)?
[x] Authorship: Has the submitting author (@uvilla) 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 @boehmc

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 (2.1.0)?
[x] Authorship: Has the submitting author (@uvilla) 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 commented 6 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @IgorBaratta, 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:

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

watching

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 commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

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

lheagy commented 6 years ago

Many thanks @IgorBaratta, @boehmc for being willing to review 🎉! In the main issue thread above, there are checklists for each of you to help guide the review. Please let me know if you have any questions or if I can help clarify anything. We really appreciate if you can complete the review within the next 2 weeks. If for any reason you are delayed, please just keep us updated. Thanks!

boehmc commented 5 years ago

Hey @uvilla (and co), HippyLib is a really cool package and nicely put together! Being able to run everything in a docker image / jupyter nb is just awesome. Thanks a lot for all the effort that went into it!!

Please allow me a few more days to play around with it. I will finish everything by the end of the week.

In the mean time I have a few minor things:

Could you add a doi for the DOLFIN paper (10.1145/1731022.1731030)?
The doi's of the bibtex book items do not appear in the pdf output. I think this could be an issue with the bib style. But I don't know whether it makes sense to include them...
Do you know the package nbval to test jupyter notebooks (https://nbval.readthedocs.io/en/latest/)? If I am not mistaken you have set up a CI on travis that runs all the notebooks, but it doesn't check the output. Using nbval is quite handy for this and maybe something you may consider in the future.
I found a few typos in the jupyter notebook (nothing important). I could either turn this into a pull request or just send you the git diff. Please let me know what you prefer.

Paper
I think the term "PDE-constrained" might be more common than "PDE-based"
The use of the word automatic in the comment on gradient and Hessian evaluations might be a bit misleading / contradicting when looking at the third and fifth paragraph. If I understood correctly, the user needs to provide the weak form of all forward / adjoint equations and FEniCS will automatically set up and solve the resulting PDEs, whereas in the future you might even skip the necessity of specifying the weak form of the adjoint equations, because AD techniques will automatically generate it for the user. Maybe you could slightly reformulate this sentence.
Since you are using Steihaug-CG, I was wondering if there is actually a need for Armijo back-tracking? In my experience, Steihaug-CG is often combined with a trust-region globalization.
"low rank factorization of the Hessian misfit" sounds a bit odd to me. Maybe Hessian of the misfit term or just Hessian?
I don't know if there are any space limitations, but I would consider adding references to part 1 and 2 of the SIAM papers of your co-authors on the "Computational framework for infinite-dimensional Bayesian inverse problems. For people not familiar with this field, this would be an excellent starting point to get a better idea of all the math that is going on in the background.

uvilla commented 5 years ago

Hi @boehmc ,

Thank you very much for taking the time to look at the code and for your spot on comments. I am happy that you found hIPPYlib really cool and the docker image easy to use.

I agree with all your comments on the paper, and I will revise it accordingly. In particular, thank you for catching the misleading statement about automatic differentiation. hIPPYlib uses FEniCS's symbolic differentiation of weak forms, but we do not use any of the automatic differentiation tools in dolfin-adjoint. I will fix that. Trust-region globalization was actually implemented recently in hIPPYlib. I will mention that in the revised paper. Overall, in my experience, the two types of globalization perform quite similarly. I will also add the references you mentioned and change the odd wordings .Missing DOIs should get fixed once I recompile the paper.
Thank you also for suggesting nbval to check the notebook output for correctness. I'll look into it.
Finally, thank you for finding some typos in the notebooks and fixing them. If it is not too much work on your hand, a pull request is very appreciated ;)

No rush to complete the review. Please take your time experimenting with the code.

Cheers,

Umberto

uvilla commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

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

uvilla commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

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

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

0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 100 14 0 14 0 0 192 0 --:--:-- --:--:-- --:--:-- 194 Error reading bibliography ./paper.bib (line 35, column 3): unexpected "d" expecting space, ",", white space or "}" Error running filter pandoc-citeproc: Filter returned error status 1 Looks like we failed to compile the PDF

uvilla commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

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

uvilla commented 5 years ago

@boehmc ,

Thank you again for your constructive comments regarding the paper. Commit 1b862f03d633a0da8eb1ab62d4d3b62d4da22563 addresses them.

Here, the revised manuscript.

Looking forward for the rest of your review.

uvilla commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

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

boehmc commented 5 years ago

@uvilla,

I've just sent you a pull requests with a few tiny typos I noticed while reviewing the paper (only one of them is actually math related)

Additionally, in tutorial 4, you may want to switch the notation of the domain from D to Omega and I think the forward operator is sometimes denoted by F and sometimes by B.

Thanks for updating the paper. Everything looks fine to me and it is very convenient to use with the docker image. This is a really nice package and I have a few applications in mind, where I might use hippylib for my own research. Thanks a lot for making it available!

uvilla commented 5 years ago

@boehmc,

Thank you very much for your pull request to fix some typos in the tutorials.

I have also updated tutorial 4 (see commit 4ff5327573049641963c8f297212b9c1db00ebfa ), to clarify that F is the parameter-to-observable map and B the observation operator.

IgorBaratta commented 5 years ago

Hi @lheagy and @uvilla, Sorry for the delay. I'll complete the review by Thursday.

IgorBaratta commented 5 years ago

I had known hIPPYlib for some time and recently played with the tutorials. The tutorials are informative and indeed an excellent place to start to learn about the package.

Great work @uvilla and colleagues!

Some small suggestions to the software paper 1st paragraph:

Replace the first parenthesis with a comma to improve readability.
Include a reference to the PETSc library, to be consistent with the other dependencies that have been credited.

8th paragraph:

correct a small typo: rectagular – rectangular

I could not find any community guidelines on the Github repository nor the Github page (https://hippylib.github.io/). If this information is not available, I would recommend creating a simple guideline on how to contribute to the package, report issues, get support, etc..

uvilla commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

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

uvilla commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

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

uvilla commented 5 years ago

@IgorBaratta thank you for your review and kind comments.

Some small suggestions to the software paper

I have made the suggested changes see commit be26f75b66399c9bb6a00238cc3fe71e0a80326e.

I could not find any community guidelines on the Github repository nor the Github page.

You are correct there is no community guidelines yet. I am drafting a CONTRIBUTING.md that will be added to the source code and published---together with the sphinx documentation--- on readthedoc. I'll let you know when this task is completed.

Thank you again,

Umberto

uvilla commented 5 years ago

@IgorBaratta,

I added contributing guidelines both in the GitHub repository (see CONTRIBUTING.md) and on readthedocs (see https://hippylib.readthedocs.io/en/latest/contributing.html).

Please let me know if there are other issues you’d like to be addressed before accepting hIPPYlib.

uvilla commented 5 years ago

@IgorBaratta ,

Did you have a change to review our contributing guidelines? Is there any other issue that you'd like to be address? Thank you again for your time in reviewing hIPPYlib and for your constructive comments.

lheagy commented 5 years ago

Hi @IgorBaratta, would you mind taking a look at the community guidelines? This is the last outstanding item on the review. Many thanks!

IgorBaratta commented 5 years ago

Hi @lheagy and @uvilla ,

There are no other issues that I would like to be addressed. Everything looks great now. Regarding the community guidelines, they are well written and complete. Congrats.

-- Ps: Sorry for the delay, I am attending a conference in China and I didn't have access to my email and github.

uvilla commented 5 years ago

@IgorBaratta and @boehmc, Thank you very much for your time and constructive comments.

@lheagy, Thank you for serving as editor for my submission. Today I’ll tag the current state of the repository as release 2.1.1, and after that we’ll be ready to “publish” the manuscript.

Thanks again to everyone.

lheagy commented 5 years ago

Excellent, many thanks @IgorBaratta and @boehmc for your review!

@uvilla, could you please archive the software on zenodo or similar and post the doi here? Thanks!

uvilla commented 5 years ago

@lheagy ,

The Zenodo DOI for the lastest hippylib release 2.1.1 (which includes Igor's and Christian's comments) is 10.5281/zenodo.1469675.

The all-versions DOI is 10.5281/zenodo.596931.

Thank you again to you, @IgorBaratta, @boehmc for reviewing my code. I truly believe that your reviewed improved the code and how users will interact with hIPPYlib.

lheagy commented 5 years ago

@whedon set 10.5281/zenodo.596931 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.596931 is the archive.

lheagy commented 5 years ago

@arfon: this submission is ready for publication. Congratulations @uvilla!! :tada:

arfon commented 5 years ago

@whedon accept

whedon commented 5 years ago

Attempting dry run of processing paper acceptance...

whedon commented 5 years ago

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

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/31, 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 commented 5 years ago

@whedon accept deposit=true

whedon commented 5 years ago

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

whedon commented 5 years ago

🚨🚨🚨 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 :point_right: https://github.com/openjournals/joss-papers/pull/33
Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.00940
If everything looks good, then close this review issue.
Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

uvilla commented 5 years ago

@lheagy and @arfon,

I checked the final PDF and everything looks good. Again, thank you very much for handling my submission.

arfon commented 5 years ago

@IgorBaratta, @boehmc - many thanks for your reviews here and to @lheagy for editing this submission ✨

@uvilla - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00940 :zap: :rocket: :boom:

whedon commented 5 years ago

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

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

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

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=

openjournals / joss-reviews