Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
36 tasks done
whedon opened this issue Sep 12, 2018 · 49 comments
Closed
36 tasks done
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Sep 12, 2018

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

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:

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

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

Please try and complete your review in the next two weeks

Review checklist for @IgorBaratta

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (2.1.0)?
  • Authorship: Has the submitting author (@uvilla) 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 @boehmc

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (2.1.0)?
  • Authorship: Has the submitting author (@uvilla) 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
Copy link
Author

whedon commented Sep 12, 2018

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

⭐ Important ⭐

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

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

watching

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

notifications

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

@whedon commands

@whedon
Copy link
Author

whedon commented Sep 12, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Sep 12, 2018

@lheagy
Copy link
Member

lheagy commented Sep 12, 2018

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
Copy link

boehmc commented Sep 26, 2018

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
Copy link

uvilla commented Sep 26, 2018

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
Copy link

uvilla commented Sep 27, 2018

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Sep 27, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Sep 27, 2018

@uvilla
Copy link

uvilla commented Sep 27, 2018

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Sep 27, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Sep 27, 2018

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
Copy link

uvilla commented Sep 27, 2018

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Sep 27, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Sep 27, 2018

@uvilla
Copy link

uvilla commented Sep 27, 2018

@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
Copy link

uvilla commented Oct 2, 2018

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Oct 2, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Oct 2, 2018

@boehmc
Copy link

boehmc commented Oct 2, 2018

@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
Copy link

uvilla commented Oct 3, 2018

@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
Copy link

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

@IgorBaratta
Copy link

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
Copy link

uvilla commented Oct 3, 2018

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Oct 3, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Oct 3, 2018

@uvilla
Copy link

uvilla commented Oct 3, 2018

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Oct 3, 2018

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Oct 3, 2018

@uvilla
Copy link

uvilla commented Oct 10, 2018

@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
Copy link

uvilla commented Oct 11, 2018

@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
Copy link

uvilla commented Oct 18, 2018

@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
Copy link
Member

lheagy commented Oct 21, 2018

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

@IgorBaratta
Copy link

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
Copy link

uvilla commented Oct 23, 2018

@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
Copy link
Member

lheagy commented Oct 23, 2018

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
Copy link

uvilla commented Oct 23, 2018

@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
Copy link
Member

lheagy commented Oct 23, 2018

@whedon set 10.5281/zenodo.596931 as archive

@whedon
Copy link
Author

whedon commented Oct 23, 2018

OK. 10.5281/zenodo.596931 is the archive.

@lheagy
Copy link
Member

lheagy commented Oct 23, 2018

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

@arfon
Copy link
Member

arfon commented Oct 23, 2018

@whedon accept

@whedon
Copy link
Author

whedon commented Oct 23, 2018

Attempting dry run of processing paper acceptance...

@whedon
Copy link
Author

whedon commented Oct 23, 2018

Check final proof 👉 openjournals/joss-papers#31

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#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
Copy link
Member

arfon commented Oct 23, 2018

@whedon accept deposit=true

@whedon
Copy link
Author

whedon commented Oct 23, 2018

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

@whedon
Copy link
Author

whedon commented Oct 23, 2018

🚨🚨🚨 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 👉 Creating pull request for 10.21105.joss.00940 joss-papers#33
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.00940
  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...

@uvilla
Copy link

uvilla commented Oct 23, 2018

@lheagy and @arfon,

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

@arfon
Copy link
Member

arfon commented Oct 23, 2018

@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 ⚡ 🚀 💥

@arfon arfon closed this as completed Oct 23, 2018
@whedon
Copy link
Author

whedon commented Oct 23, 2018

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

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

6 participants