[REVIEW]: MUQ: The MIT Uncertainty Quantification Library

whedon commented 3 years ago

Submitting author: @mparno (Matthew Parno) Repository: https://bitbucket.org/mituq/muq2.git Version: 0.4.1 Editor: @pdebuyl Reviewers: @martinmodrak, @georgiastuart Archive: 10.5281/zenodo.5770267

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

Status badge code:

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

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) by leaving comments 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

@martinmodrak, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @pdebuyl know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @georgiastuart

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

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] Contribution and authorship: Has the submitting author (@mparno) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

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: Does the paper have a section titled 'Statement of Need' that clearly states 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 functionality 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] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[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] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @martinmodrak

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

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] Contribution and authorship: Has the submitting author (@mparno) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

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: Does the paper have a section titled 'Statement of Need' that clearly states 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 functionality 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] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[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] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @martinmodrak it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pdebuyl commented 3 years ago

@whedon add @georgiastuart as reviewer

whedon commented 3 years ago

OK, @georgiastuart is now a reviewer

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=6.26 s (159.0 files/s, 27439.9 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
C/C++ Header                    366          12654          19594          49204
C++                             353          11682           6172          37366
Python                           76           2494           1609           7464
CMake                           108           1206            813           4382
reStructuredText                 32           2450           2778           4153
SVG                               3              0              0           1186
D                                 1              0              0            961
Jupyter Notebook                  8              0           2901            807
YAML                             23             41             30            557
TeX                               3             45              0            304
Markdown                          8             73              0            277
XML                               1              5              7            170
Dockerfile                        4             28             14             91
Bourne Shell                      3             32             19             69
HTML                              1              3             16             46
INI                               1              0              0             19
CSS                               1              0              0             11
TOML                              2              0              0              6
Bourne Again Shell                1              0              0              2
--------------------------------------------------------------------------------
SUM:                            995          30713          33953         107075
--------------------------------------------------------------------------------

Statistical information for the repository '8116a45d3372a921a3269c25' was
gathered on 2021/03/02.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Alexandra Datz                   3            77             20            0.03
Andrew                          85         13805           8030            7.40
Andrew Davis                   108         19392           8923            9.59
Andy Davis                      15          1536            430            0.67
Arnold Song                     15          1984           2350            1.47
Brendan West                    15          1699            165            0.63
Devin O'Connor                  13           963            254            0.41
Hodgdon, Taylor S ER             8           828             97            0.31
Josephine Westermann             5           179             84            0.09
Ki-Tae Kim                       2             8              2            0.00
Linus Seelinger                116         26050          17873           14.88
Matt                            25          3778            568            1.47
Matt Parno                      11          2169            443            0.89
Matthew Parno                  383         97268          32459           43.96
Max Liu                          1             1              0            0.00
andyddavis                       1            53              6            0.02
cassielumbrazo                   3            92             42            0.05
davisad                         66         49707           3611           18.07
vagrant                          1           156              0            0.05

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Alexandra Datz               69           89.6          4.3                0.00
Andrew                     6323           45.8         45.0               24.92
Andrew Davis               8626           44.5         36.4               17.12
Arnold Song                1003           50.6         31.6               34.20
Brendan West                370           21.8         35.5               51.62
Devin O'Connor              628           65.2         33.6                7.32
Hodgdon, Taylor S ER        533           64.4         32.6                1.13
Josephine Westermann        179          100.0          3.4               29.61
Ki-Tae Kim                    8          100.0          8.2                0.00
Linus Seelinger            6133           23.5         18.0               10.19
Matt                       3740           99.0         15.1               22.19
Matt Parno                  293           13.5         47.6               14.33
Matthew Parno             71297           73.3         31.1               16.22
Max Liu                       1          100.0         18.7                0.00
andyddavis                   53          100.0         11.1               16.98
cassielumbrazo               63           68.5         42.4               22.22
davisad                   48920           98.4         49.7               23.33

pdebuyl commented 3 years ago

@georgiastuart @martinmodrak make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page.

The review process will happen in this issue page, so questions to the author or to me can be added as comments here.

In a JOSS review, you can query the authors directly here without the delay of the editorial office. Given that the codebase is large, it is good to keep it in mind as you go.

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1137/130915005 is OK
- 10.1137/19M126966X is OK
- 10.1287/opre.1070.0496 is OK

MISSING DOIs

- 10.1016/j.jcp.2015.10.008 may be a valid DOI for title: Dimension-independent likelihood-informed MCMC
- 10.1214/13-sts421 may be a valid DOI for title: MCMC methods for functions: modifying old algorithms to make them faster
- 10.1615/int.j.uncertaintyquantification.2019027384 may be a valid DOI for title: Embedded model error representation for Bayesian model calibration
- 10.1137/140964023 may be a valid DOI for title: Randomize-then-optimize: A method for sampling from posterior distributions in nonlinear inverse problems
- 10.1137/120890715 may be a valid DOI for title: Adaptive Smolyak pseudospectral approximations
- 10.1137/16m1084080 may be a valid DOI for title: Parallel local approximation MCMC for expensive models
- 10.1137/17m1134640 may be a valid DOI for title: Transport map accelerated Markov chain Monte Carlo

INVALID DOIs

- None

pdebuyl commented 3 years ago

@georgiastuart I forgot to assign you in the pre-review, sorry. If there is any issue let me know (especially if you don't receive an invitation to JOSS' reviewer group which is necessary to proceed).

pdebuyl commented 3 years ago

@mparno can you fix all the DOIs already ? This will be necessary anyway before acceptance. Also, some high-level information such as the use of pybind for the C++ / Python binding is welcome. You can also list some examples from the extensive list that is provided with the software (no code illustration but simple mention that the capabilities are well demonstrated is useful).

georgiastuart commented 3 years ago

@pdebuyl I got the invite and I'm good to go. As you mentioned in the pre-review issue, I'm fine with focusing on the python interface and @martinmodrak can focus on the C++ interface.

mparno commented 3 years ago

@mparno can you fix all the DOIs already ? This will be necessary anyway before acceptance. Also, some high-level information such as the use of pybind for the C++ / Python binding is welcome. You can also list some examples from the extensive list that is provided with the software (no code illustration but simple mention that the capabilities are well demonstrated is useful).

@pdebuyl Of course. I'll work on this today. Thank you for your feedback.

mparno commented 3 years ago

@pdebuyl Reference DOIs have been updated and additional information about our examples and python bindings have been added the summary section of the paper. Everything is up to date in the master branch of MUQ's repository.

georgiastuart commented 3 years ago

@mparno Is Boost necessary for installation (by building the software), or is it just using Eigen? If just using Eigen, would it be possible to remove the boost dependency as it seems unnecessary and boost is hefty. I ask because boost is listed in the same line as Eigen in your dependency table.

Edit: nevermind, answered my own question by looking at the code and I see you're using other boost functionality. I would recommend splitting boost and eigen onto two lines if you think that makes sense for your documentation.

mparno commented 3 years ago

@georgiastuart Thanks for pointing this out. Yes, we require Boost (as you figured out). There must be a bug in the doxygen for producing that table of dependencies. Boost and Eigen should be separated. I'll work on fixing that later today. Thanks!

Update: The dependency table has been fixed.

pdebuyl commented 3 years ago

@whedon check references

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.jcp.2015.10.008 is OK
- 10.1214/13-STS421 is OK
- 10.1615/Int.J.UncertaintyQuantification.2019027384 is OK
- 10.1137/140964023 is OK
- 10.1137/130915005 is OK
- 10.1137/19M126966X is OK
- 10.1137/120890715 is OK
- 10.1137/16M1084080 is OK
- 10.1137/17M1134640 is OK
- 10.1287/opre.1070.0496 is OK

MISSING DOIs

- None

INVALID DOIs

- None

pdebuyl commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pdebuyl commented 3 years ago

@georgiastuart @martinmodrak gentle reminder

whedon commented 3 years ago

:wave: @martinmodrak, please update us on how your review is going (this is an automated reminder).

pdebuyl commented 3 years ago

Please ignore the bot, it often picks a wrong time for the automated reminder.

martinmodrak commented 3 years ago

I made some attempts at installing today, reached out on the MUQ slack for some help. I should be able to dedicate some time for at least an initial round of review this week (provided I get to resolve the installation issues).

georgiastuart commented 3 years ago

Working on this submission this week.

mparno commented 3 years ago

I made some attempts at installing today, reached out on the MUQ slack for some help. I should be able to dedicate some time for at least an initial round of review this week (provided I get to resolve the installation issues).

@martinmodrak Sorry you ran into these issues using the conda package. I finally pushed up a fix to conda-forge last night that should allow you to seamlessly use the conda package with c++. Thanks for helping identify the issue!

martinmodrak commented 3 years ago

So I managed to get a bit more in-depth with the software. There is a lot of work done and I didn't encounter any blocking issues when inspecting the sampling part. I believe this is a useful piece of software. The biggest outstanding problem in that part IMHO is that there is basically no support to check convergence of MCMC (and indeed some of the examples fail convergence checks) as reported at https://bitbucket.org/mituq/muq2/issues/20/no-convergence-diagnostics-for-mcmc . I did start a few other issues for minor stuff, but none of it is IMHO important for acceptance.

Some further notes that didn't fit into issues:

Docs:

I find the documentation borderline usable (i.e. that I would not object to acceptance of the work for JOSS if the docs were not improved, but I am also not very happy about it). One needs to read code to complement documentation (luckily, the code is linked from the docs, so this is easy and the code is quite readable - so as I said, mostly enough for a user like me, but not great). E.g. SampleCollection::WriteToFile says it writes the samples to file. But I need to check the code to see that it writes to a HDF5 file and stores the data in “samples” and “weights” groups within the file. Similarly for SampleCollection::RunningExpectedValue - returns a std::vector< Eigen::VectorXd > and I need to check the code to get which dimension is samples and which is the quantities.

Contributors:

I noticed in the BitBucket repo that the code has some (presumably minor) contribution from Alexandra Datz, who is not mentioned in the paper. Maybe mentioning them (and possible other people who contributed some small pieces of code which I missed) in acknowledgement would be reasonable? But I obviously know very little about the context, so if there are good reasons not to do so, I do not insist.

Personal opinions:

There are also a few things that I don't think I am capable of impartially assessing (primarily because of my affiliation with Stan) and are definitely opinion-based, but I wanted to mention - addresssing those is by no means a requirement for acceptence (at least not for me).

Citing libraries: The code builds on a bunch of open source libraries (Eigen, Sundials, libnlopt, Stan math). At least some of those AFAIK depend on academic funding for development, so citing them in the paper would probably be sensible.

Comparison to Stan: The paper mentions that Stan is "not suitable for large-scale models" - with the implied contrast that MUQ2 is. I think some care is needed here: first "large-scale" can mean many different things. Stan can definitely sample some real-life models with tens of thousands of parameters in a reasonable time and we are quite sure it does so correctly (or loudly signals there is an issue). Is this large-scale enough? Or is "large-scale" supposed to refer to the amount of code needed to write the likelihood (where definitely the need to convert to Stan could be a significant issue). Further, MUQ2 doesn't list (anywhere I could easily find) any use case where an actually large model (in any sense of the word) is succesfully tackled. As I mentioned above - at least for some of the algorithms MUQ2 provides there is not currently a very strong guarantee that they actually produce a correct answer once the computation finishes. So just showing that MUQ2 computes something while Stan fails/is impractical doesn't necessarily mean that MUQ2 is better for that use case.

As I said, I don't think MUQ2 needs to compete with Stan in any way - it has different audience, different interface and different set of features and this is great. Just that the passage in the paper comparing to other tools feels a bit like the usual academic boilerplate of "we have to say our work is novel/best/different or it won't be publishable" which I hope is not needed for JOSS. On the other hand, if there is an actual use case where you want to claim superiority to Stan (or others), then I think the performance should be actually demonstrated.

georgiastuart commented 3 years ago

Update from me: I'm working on my review still and will post ASAP.

martinmodrak commented 3 years ago

Also should have mention, that all of the suggestions (and the issues I filed) are open to discussion. There are very few things I feel particularly strongly about and I definitely don't want the authors to contort themselves to satisfy my whims :-)

pdebuyl commented 3 years ago

Thank you @martinmodrak for the review! It is phrased in a very constructive manner.

It is indeed appropriate to cite the tools on which a JOSS paper builds.

pdebuyl commented 3 years ago

@mparno you can already take into account @martinmodrak 's feedback. If you act on the issues that he open at your repository, I would appreciate a notification here to follow the developments.

mparno commented 3 years ago

@martinmodrak Thanks for this great feedback. It is very helpful for our team. I'm working with the other authors to address these the issues you brought up (and created on the MUQ2 repository). I will let you and @pdebuyl know when MUQ is ready for another look.

georgiastuart commented 3 years ago

So sorry this took so long. Here is my review. As agreed, I primarily looked at the python side of things.

General Thoughts

Overall, very nice package. I enjoyed looking at it and I can see myself using this package as a UQ framework in my own research.

Here's the overview of my concerns/requests:

I think documentation on whether the docker containers work with Singularity would be very helpful. Since this package contains parallel capabilities, it's likely someone will want to use it on an HPC system (me!)
- On this note, parallel documentation is lacking. It was unclear to me without digging into the source code how the code was parallelized (mpi, whatever). I also don't know if the parallel capabilities can be used with the python interface. The only parallel examples are C++ only.
Documentation in general is lacking and needs to be expanded substantially.
- More detailed "getting started" section that guides the user through their first project
- high level discussion of the modules would be nice
- Maybe incorporate some of the text in the excellent examples into the docs?

Installation

Installation via conda was straightforward and I did not run into any issues.

Docker containers have sufficient documentation on how to use. However:

The hippylib container is very old (3 years). Is it still usable? Will it be updated in the future?
Some documentation on how to use with singularity would be helpful since that is the only way to use containers on most HPC systems. Has MUQ been tested with examples needing HPC support? Would love to see these examples if available.

Documentation

I found the documentation confusing and hard to navigate.
The "Descriptions" page seems to be just the docker containers and a duplicate of what's in the docker container page. Delete?
Like @martinmodrak, I found the documentation almost unusable without the examples. I would recommend adding a section that is a high-level overview of what each module does and a more detailed "getting started" page.
- At the very least, a comprehensive list and brief description of the algorithms implemented would be great.
That being said, the examples are excellent and well documented, in particular the python notebooks that I primarily looked at.

Usage

My complaints here are mainly with the murky situation with the parallelization. However, I approached this with my area in mind (geophysical inverse problems) which have highly expensive forward problems that absolutely need to be parallelized, so if this is outside the scope of the package please disregard.
- It is unclear whether we can use parallelized forward models with MUQ
- It is unclear whether MUQ is parallelized at the chain level (eg one chain per process) or if a single chain can be parallelized
- It's also unclear if the python interface works with mpi4py
The most useful example to me was "Analyzing Groundwater Pump Tests with Hierarchical Models." Very well done and it answered a lot of my questions.
The paper states "its primary focus is on the solution of Bayesian inverse problems with computationally expensive models and potentially high dimensional parameter spaces." Would love to see an example that illustrates MUQ best practices for this.
- I'd especially like to see an example that uses an external python library to drive the forward problem (my use case).
- If that's asking for the moon, though, it's not absolutely necessary for passing my review :)

Paper

"make it more difficult for users to expose efficient gradient evaluation techniques for high dimensional problems." I've run into this exact situation trying to use Stan and PyMC3.
Paper is good and I think outlines why MUQ is useful quite well.
However, I'd like to see more discussion of the paper claims, especially the bit about using any physics based forward model, in the documentation.

georgiastuart commented 3 years ago

I'd also like to add that the authors are active on their slack and very quick to respond to questions. Good support!

pdebuyl commented 3 years ago

Thanks @georgiastuart for the review!

pdebuyl commented 3 years ago

Hi @mparno any update? Now that the two reviews are in you can of course reply to everything in one iteration.

mparno commented 3 years ago

@georgiastuart and @martinmodrak Thank you both for your reviews. We've started addressing some of your concerns and will get back to you all with a longer response as soon as we can. @pdebuyl We will keep you posted on our progress. Thanks!

pdebuyl commented 3 years ago

Hi @mparno any news?

mparno commented 3 years ago

@pdebuyl Thanks for checking in. Our team is slowly working on revisions, but it’s going slowly due to some job changes and PhD defenses amongst the authors. Unfortunately I don’t expect us to have a complete response addressing all of the important suggestions from the reviewers until June. Is that timeline OK with you?

pdebuyl commented 3 years ago

Hi @mparno it is ok.

pdebuyl commented 3 years ago

Hi @mparno any news?

pdebuyl commented 3 years ago

@mparno gentle reminder.

linusseelinger commented 3 years ago

@pdebuyl Sorry for the delays, seems like we all are a bit too busy on our side... We are making progress though. Changes to the paper as requested should be completed soon (references to dependencies, improved comparison to other codes, references to large-scale models in MUQ, acknowledgement of minor contributions from community). A good portion of the code issues raised by the reviewers is addressed as well, and corresponding improvements merged. The biggest remaining issue for us is improving the doxygen documentation. We can't afford a larger overhaul right now due to time constraints. Would that be considered a road block for the JOSS publication?

mparno commented 3 years ago

@pdebuyl My apologies for being out of touch. I've been transitioning between jobs and haven't been able to keep up with everything. That said, I am working exclusively on this for the next week and we should be able to pull together a complete response by the end of the month.

@linusseelinger I'm focusing on the doxygen. It's not a complete overhaul, but a significant reorganization with some major additions that should address the concerns that the reviewers brought up.

pdebuyl commented 3 years ago

Hi @linusseelinger and @mparno , thanks for the update.

Regarding roadblocks, I don't see any but there are a number of review items from the reviewers which require either a change in the code/doc or in the paper and those should be addressed.

About some specific comments from the reviewers: JOSS does not require "novelty" in the traditional sense:

Your software should be a significant contribution to the available open source software that either enables some new research challenges to be addressed or makes addressing research challenges significantly better (e.g., faster, easier, simpler).

Performance claims, however, must be illustrated by the authors explicitly.

martinmodrak commented 3 years ago

@linusseelinger : From my side the docs is not a roadblock, as I wrote in the review:

I find the documentation borderline usable (i.e. that I would not object to acceptance of the work for JOSS if the docs were not improved, but I am also not very happy about it).

Good luck with improving the project!

mparno commented 3 years ago

Thanks again to @martinmodrak and @georgiastuart for their careful reviews. We have incorporated most of your suggestions and it has definitely made MUQ a better project. The current version v0.3.4 of MUQ contains all of our updates, which we're currently using to update our conda package and docker images. Below we also provide individual responses to the main points brought up in the review. We look forward to continuing to discuss MUQ with you and are happy to address any additional concerns that might arise.

We have not yet addressed some of the important parallelization questions that @georgiastuart raised. @georgiastuart brings up many good points and addressing them will undoubtedly make MUQ a better and more capable package, but we feel that this lies outside the scope of our current submission. Given the large size of MUQ, we would like to ask @pdebuyl if, instead of increasing the size of our current submission, it would be preferable to resubmit the parallel components of MUQ in a new JOSS submission. Parallelization in MUQ depends heavily on our companion project ParCer and we believe their would be "substantial scholarly effort" in the parallel components alone. Of course, we are open to whatever @pdebuyl and the editorial staff think is the best option.

Specific Review Responses

GetSamples and GetQOIs return nullptr.

In both SLMCMC and GreedyMLMCMC the GetSamples() and GetQOIs methods return null. This should be at least mentioned in the documentation, but I don’t really understand why at least for SLMCMC I cannot just get single_chain->GetSamples() - at least when writing to file or computing the means, this is exactly what I get…

This has been fixed and merged into the master branch.

Lack of convergence diagnostics

The biggest outstanding problem in that part IMHO is that there is basically no support to check convergence of MCMC (and indeed some of the examples fail convergence checks) as reported at https://bitbucket.org/mituq/muq2/issues/20/no-convergence-diagnostics-for-mcmc.

We have added the split-Rhat and rank-normalized statistics to a new "Diagnostics" capability.
The convergence diagnostics have also been added to our new MCMC documentation page as well as some of the introductory examples.

Add information on how to compile examples.

It seems that expected/recommended way to compile and run the C++ examples on the web is to actually clone/download the repo and then use cmake + make for build with the provided configuration files. The example pages should mention that (I spent a bunch of time trying to compile the CPP on my own before finding this advice on the Slack channel).

We have added a "Link to MUQ" section on the installation page with more detailed instructions on how to create an executable that links against the MUQ libraries.

Issue 22: Installation instructions should mention dependencies.

https://bitbucket.org/mituq/muq2/issues/22/installation-instructions-should-mention

This has been added to our doxygen-generated documentation. See this page.

Utility of documentation

I find the documentation borderline usable (i.e. that I would not object to acceptance of the work for JOSS if the docs were not improved, but I am also not very happy about it). One needs to read code to complement documentation (luckily, the code is linked from the docs, so this is easy and the code is quite readable - so as I said, mostly enough for a user like me, but not great).

Like @martinmodrak, I found the documentation almost unusable without the examples. I would recommend adding a section that is a high-level overview of what each module does and a more detailed "getting started" page.

The MUQ documentation has been completely reorganized and a substantial amount of new material has been added. Please see our documentation website for the updates, especially the various chapters for the MCMC and Modeling modules. Following suggestions from @martinmodrak and @georgiastuart, high level overviews and "quickstarts" have been added for each of MUQ's main modules. The documentation for individual functions, such as SampleCollection::WriteToFile and SampleCollection::RunningExpectedValue, has also been improved.

Comparison with STAN in paper

As I said, I don't think MUQ2 needs to compete with Stan in any way - it has different audience, different interface and different set of features and this is great. Just that the passage in the paper comparing to other tools feels a bit like the usual academic boilerplate of "we have to say our work is novel/best/different or it won't be publishable" which I hope is not needed for JOSS.

We have reworded the paper to better represent the different types of problems that Stan and MUQ are designed for.

Using any physics-based forward model

However, I'd like to see more discussion of the paper claims, especially the bit about using any physics based forward model, in the documentation.

The discussion of our model interface has been improved in the paper to help demonstrate how physics-based model can be used within MUQ.

Contributors listed in paper

I noticed in the BitBucket repo that the code has some (presumably minor) contribution from Alexandra Datz, who is not mentioned in the paper. Maybe mentioning them (and possible other people who contributed some small pieces of code which I missed) in acknowledgement would be reasonable? But I obviously know very little about the context, so if there are good reasons not to do so, I do not insist.

Indeed, there are many small contributions from the broader MUQ community. We have added a more inclusive acknowledge statement to the paper to capture this.

Computationally Expensive Models

The paper states "its primary focus is on the solution of Bayesian inverse problems with computationally expensive models and potentially high dimensional parameter spaces." Would love to see an example that illustrates MUQ best practices for this.

We have added references in the paper pointing at largescale applications that use MUQ. This should give users a feel for what's possible without having to distribute large models within MUQ itself.
In terms of implementing your own model, our diffusion equation example might be relevant. It uses finite differences to solve an elliptic PDE to show how a more complicated model might be implemented in MUQ. This, combined with our improved documentation of the Modeling module, should give you an idea of best practices for model definitions in MUQ.

Citing libraries

The code builds on a bunch of open source libraries (Eigen, Sundials, libnlopt, Stan math). At least some of those AFAIK depend on academic funding for development, so citing them in the paper would probably be sensible.

This has been added to the paper.

HIPPylib Docker Container

The hippylib container is very old (3 years). Is it still usable? Will it be updated in the future?

The container is in the process of being updated.

pdebuyl commented 3 years ago

Hello @mparno thank you for the update. Regarding the inclusion of new features, it is not necessary. The most important is that the description of the actual capabilities be of sufficient clarity. On the principle of another paper, it is decided upon submission and I can't say already what would be the answer. If it about extending the "existing in C++" parallel capability to the Python interface, my guess is that it would be insufficient for a separate submission though. Note that you can keep on asking citation of the first JOSS paper for the software anyway.

@georgiastuart @martinmodrak can you check the changes and update your reviews accordingly?

martinmodrak commented 3 years ago

Thanks for the updates. Unfortunately, I am leaving for a two weeks vacation on Friday and won't be able to look into this before returning. I'll try to get to this ASAP after returning, but a realistic date for an update from me is around July 29th. Sorry for the delay.

mparno commented 3 years ago

Thanks for clarifying @pdebuyl. We'll hold off on the other paper for now.

Enjoy your vacation @martinmodrak!

pdebuyl commented 3 years ago

Hi @martinmodrak and @georgiastuart can you update your review?

martinmodrak commented 3 years ago

@whedon generate pdf

openjournals / joss-reviews