search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: UM-Bridge: Uncertainty quantification and modeling bridge #4748

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@linusseelinger<!--end-author-handle-- (Linus Seelinger) Repository: https://github.com/UM-Bridge/umbridge Branch with paper.md (empty if default branch): Version: v1.2.2 Editor: !--editor-->@pdebuyl<!--end-editor-- Reviewers: @georgiastuart, @Himscipy Archive: 10.5281/zenodo.7743819

Status

status

Status badge code:

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

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

@georgiastuart & @Himscipy, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

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 ✨

Checklists

📝 Checklist for @georgiastuart

📝 Checklist for @Himscipy

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.14 s (403.1 files/s, 269127.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                     4           5306           7381          21414
Python                          11            221             43            793
YAML                            17             47              7            645
Markdown                         7            252              0            494
R                                4             46             99            182
C++                              3             44             19            132
TeX                              1              9              0             99
Bourne Shell                     4             21             21             58
Dockerfile                       4             14              0             27
CMake                            1              5              0              8
-------------------------------------------------------------------------------
SUM:                            56           5965           7570          23852
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 674

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.03076 is OK
- 10.1146/annurev.fluid.010908.165248 is OK
- 10.1063/1.1699114 is OK
- 10.1145/3458817.3476150 is OK
- 10.1007/978-3-319-11259-6_23-1 is OK
- 10.4208/cicp.2009.v6.p826 is OK
- 10.7287/PEERJ.PREPRINTS.1686V1 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 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 2 years ago

Hi @georgiastuart , @Himscipy ,

During the review, feel free to ask questions here to the authors directly if you need clarifications, or to me regarding the editorial part. If you open issues on the code's repository, please report them and/or their resolution here as well so that I can follow that part as well.

Thanks for reviewing for JOSS!

georgiastuart commented 2 years ago

Review checklist for @georgiastuart

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

pdebuyl commented 2 years ago

Hi @georgiastuart thanks for starting the review.

@Himscipy do you have any question regarding the editorial process?

pdebuyl commented 2 years ago

-> @georgiastuart @Himscipy gentle reminder of the review in progress.

danielskatz commented 2 years ago

👋 Hi all - as the track editor here, I just wanted to check in on on the progress of this review after a couple of weeks with no changes

georgiastuart commented 2 years ago

Thanks for the nudge @danielskatz , I'll get back to this ASAP.

georgiastuart commented 2 years ago

@linusseelinger can you explain the contributions of Matthew Parno? He's listed as an author but not listed in the contributors list on GitHub.

Himscipy commented 2 years ago

Review checklist for @Himscipy

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

georgiastuart commented 2 years ago

@linusseelinger, you can disregard the above, I found Matt's contributions in the benchmarks repo.

A few questions / comments...

  1. I'm curious whether it would be simple to implement a straightforward, password-based authentication to the server?
  2. You have several good examples, but it took a lot of clicking around to find the source code for the examples. Please link from the documentation to the appropriate directory in the benchmarks repo, or (even better) link and inline the files into the documentation. I think that's more useful than just having a mathematical explanation and output pictures.
  3. Can you add a discussion about the format expected for the parameters and config function arguments and the return values for each method in the server? Apologies if I missed this in the docs somewhere.

I intend to look at this again and try to set up an example with my own python-based model, but the above are my first impressions. Very cool work!

georgiastuart commented 2 years ago

Also, please add community guidelines in a CONTRIBUTION.md file or similar to the repository.

georgiastuart commented 2 years ago

Another question, is UM-Bridge capable of using mpi-parallelized models?

linusseelinger commented 1 year ago

@georgiastuart Thanks a lot for your comments!

  1. Authentication has not been our focus so far, we are largely working in fully trusted environments. However, I have been using authentication for running UM-Bridge model containers in a cloud environment. Here, the load balancer I'm using can check for a password/access token in HTTP headers before passing requests through to the models themselves. I believe that's the best solution, since that way the security aspect is handled by a specialized component instead of UM-Bridge itself. Do you have a specific application in mind?
  2. Good point, we'll update the docs accordingly!
  3. Those are (somewhat) mentioned in the clients docs, but I'll make the docs more consistent and add more detail to the model side.
  4. MPI models with UM-Bridge are certainly possible, we currently have two approaches in use: Either the UM-Bridge server is a thin wrapper that calls a separate MPI parallel code (this may all happen inside a container, so is transparent to the user); or UM-Bridge runs on rank zero, distributing incoming parameters via MPI broadcast. Regarding containers: MPI inside a single container just works. MPI across multiple containers (as needed for multiple hardware nodes) is not trivial, but fully supported by our kubernetes setup (you have to inherit from a specific choice of base images in that case. Beyond that, our kubernetes config takes care of everything. You can just plug in your custom image, and your code will see all MPI ranks across nodes/containers as usual).
  5. Just added a CONTRIBUTING.md.
Himscipy commented 1 year ago

Hi @georgiastuart, I accidentally edited your review checklist, my apologies for that. However, I have brought it back to the state where it was, but I would like you to cross-check it again. Thank you.

Himscipy commented 1 year ago

Hi @linusseelinger

Here few comments related to the software paper and repository;

linusseelinger commented 1 year ago

@georgiastuart Examples now link to the respective source code (turns out pulling source files directly from the repo is not trivial in our docs system, so we'll go with plan B for now). Types are now better documented, in particular for Python client and server where it's not obvious to see what is expected. The Python code also enforces types by explicitly checking, so I hope that's sufficient to guide new users. I hope that covers your comments about docs / types?

linusseelinger commented 1 year ago

@Himscipy Thank you for your feedback! I have added some tests to fully cover the C++ and Python clients beyond the examples we had. The server side should be fairly well-tested already (see protocol_conformity_1.0, which is run on every model). I'm hesitant about code coverage checks, since UM-Bridge includes a couple of different languages, and more are to come. Also, since the libraries are mostly protocol wrappers, full coverage is fairly easy to achieve. Will soon adapt the paper as requested!

pdebuyl commented 1 year ago

Hi @linusseelinger thank you for addressing the comments from both reviewers so far.

@georgiastuart @Himscipy can you assess the update?

linusseelinger commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

linusseelinger commented 1 year ago

@Himscipy Btw, last update of paper includes the improvements you requested.

pdebuyl commented 1 year ago

@georgiastuart @Himscipy can you have a look at @linusseelinger 's update ?

pdebuyl commented 1 year ago

@georgiastuart @Himscipy it is over a month since the update by the authors, may I ask you to proceed with the review?

Himscipy commented 1 year ago

Hi @pdebuyl Thank you for the reminder. I will finalize the review by COB today.

Himscipy commented 1 year ago

Hi @pdebuyl , I have looked at the latest changes and gone through the reviewers checklist for the paper. Thank you for providing the opportunity for reviewing the submission.

@linusseelinger the updated submission looks good, good luck!

pdebuyl commented 1 year ago

Hi @Himscipy thanks a lot for completing the review! I had seen your first message but not the second, sorry about this.

pdebuyl commented 1 year ago

@georgiastuart can you assess the changes since your review?

georgiastuart commented 1 year ago

Apologies, I went on maternity leave before finishing this.

I am satisfied by the response from the authors!

pdebuyl commented 1 year ago

Hi @georgiastuart thank you for completing the review!

@linusseelinger I'll proceed (not before 10 days, sorry) to the subsequent editorial steps.

pdebuyl commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

pdebuyl commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.03076 is OK
- 10.1146/annurev.fluid.010908.165248 is OK
- 10.1063/1.1699114 is OK
- 10.1145/3458817.3476150 is OK
- 10.1007/978-3-319-11259-6_23-1 is OK
- 10.4208/cicp.2009.v6.p826 is OK
- 10.7287/PEERJ.PREPRINTS.1686V1 is OK
- 10.12688/openreseurope.14445.2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
pdebuyl commented 1 year ago

Hi @linusseelinger I read the paper and tested a few of the examples. Everything ran flawlessly and I have no further comments or recommendations.

Can you

  1. Make a tagged release
  2. archive it on zenodo (make sure to use same title and authors as JOSS article)
  3. provide the doi of the zenodo archive here
linusseelinger commented 1 year ago

@pdebuyl Thanks a lot! Here's the DOI: 10.5281/zenodo.7743819

pdebuyl commented 1 year ago

@editorialbot set 10.5281/zenodo.7743819 as archive

editorialbot commented 1 year ago

Done! Archive is now 10.5281/zenodo.7743819

pdebuyl commented 1 year ago

@editorialbot set 1.2.2 as version

editorialbot commented 1 year ago

Done! version is now 1.2.2

pdebuyl commented 1 year ago

@editorialbot set v1.2.2 as version

editorialbot commented 1 year ago

Done! version is now v1.2.2

pdebuyl commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.21105/joss.03076 is OK
- 10.1146/annurev.fluid.010908.165248 is OK
- 10.1063/1.1699114 is OK
- 10.1145/3458817.3476150 is OK
- 10.1007/978-3-319-11259-6_23-1 is OK
- 10.4208/cicp.2009.v6.p826 is OK
- 10.7287/PEERJ.PREPRINTS.1686V1 is OK
- 10.12688/openreseurope.14445.2 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

:wave: @openjournals/csism-eics, this paper is ready to be accepted and published.

Check final proof :point_right::page_facing_up: Download article

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/4061, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

danielskatz commented 1 year ago

👋 @linusseelinger - I'm the track editor who will take over at this point. While proofreading the paper, I found some small changes that I think are needed: please merge https://github.com/UM-Bridge/umbridge/pull/12 or let me know what you disagree with, then we can proceed to acceptance and publication