search

JuliaCon / proceedings-review

6 stars 1 forks source link

[REVIEW]: ReactiveMP.jl: A Julia Package for Reactive Message Passing-based Bayesian Inference #91

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@bvdmitri<!--end-author-handle-- (Dmitry Bagaev) Repository: https://github.com/biaslab/ReactiveMP.jl Branch with paper.md (empty if default branch): Version: v1.3.1 Editor: Reviewers: @ludgerpaehler, @torfjelde Archive:

Status

status

Status badge code:

HTML: <a href="https://proceedings.juliacon.org/papers/d3ea9d7a33de29ee7c74f38b3cd7b43c"><img src="https://proceedings.juliacon.org/papers/d3ea9d7a33de29ee7c74f38b3cd7b43c/status.svg"></a>
Markdown: [![status](https://proceedings.juliacon.org/papers/d3ea9d7a33de29ee7c74f38b3cd7b43c/status.svg)](https://proceedings.juliacon.org/papers/d3ea9d7a33de29ee7c74f38b3cd7b43c)

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

@ludgerpaehler & @torfjelde, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @matbesancon 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 @ludgerpaehler

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ludgerpaehler, @torfjelde 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/JuliaCon/proceedings-review) 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/JuliaCon/proceedings-review:

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

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

@whedon generate pdf
whedon commented 2 years ago

PDF failed to compile for issue biaslab/ReactiveMP.jl#91 with the following error: 

 Can't find any papers to compile :-(
whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.25 s (1254.8 files/s, 245044.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                          257           4031            697          11947
SVG                              5              2              0           3392
Jupyter Notebook                30              0          33646           3160
Markdown                        10            527              0           1301
TOML                             4            266              1           1256
YAML                             3              0              0            115
-------------------------------------------------------------------------------
SUM:                           309           4826          34344          21171
-------------------------------------------------------------------------------

Statistical information for the repository 'a2dfaf8a51df1ef3252c8c3b' was
gathered on 2021/10/08.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Dmitri Bagaev                    2           552            552          100.00

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

Hey @ludgerpaehler, @torfjelde! Thank you for starting the review process. Just a little bit of remarks:

whedon commented 2 years ago

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

whedon commented 2 years ago

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

torfjelde commented 2 years ago

Just commenting to let you know I'm working on the review and haven't forgotten about it! Just re-familiarizing myself with a bit of the background before I look at the paper.

Also, @matbesancon it seems like the invitation link isn't working anymore? I'm not able to edit the issue.

torfjelde commented 2 years ago

@matbesancon I can't edit the checklist :confused:

matbesancon commented 2 years ago

I am not sure why this happens, you're the second one to have the issue. In the meantime, the hack @gdalle found was to copy the checklist to their own comment and edit it. Will let you know when we find a fix

matbesancon commented 2 years ago

for the invitation, it should be fine now since you are listed as one of the two reviewers

torfjelde commented 2 years ago

Decision

Accept as-is.

General comments

This is my first time reviewing for JOSS, so if I've done/approached this in the wrong manner, please let me know!

All in all, I think this looks like a really nice piece of work and indeed very useful for the Julia community. As far as I can tell, the paper and the documentation is extensive and correct.

With this being said, I have one minor dissatisfaction wrt. both the paper and the documentation for the package, though I don't think it's sufficient to warrant a reject or another round of reviews. It essentially boils down to the overall presentation being a bit too optimistic:

I would also mention that I'm probably overly sensitive to this because I'm part of the Julia PPL community and know that it can be quite difficult for users to navigate the space:)

But again, overall, I think ReactiveMP.jl is a really valuable addition to the community! Very well done!

Checklist

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

torfjelde commented 2 years ago

@whedon generate pdf

EDIT: I was just checking if I had misunderstood where the paper was located (I've been using the one from the juliacon branch in the repo).

whedon commented 2 years ago

PDF failed to compile for issue biaslab/ReactiveMP.jl#91 with the following error: 

 Can't find any papers to compile :-(
bvdmitri commented 2 years ago

@whedon generate pdf from branch paper/julia_proceedings_2021

whedon commented 2 years ago 
Attempting PDF compilation from custom branch paper/julia_proceedings_2021. Reticulating splines etc...
whedon 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:

torfjelde commented 2 years ago

Ah thanks @bvdmitri ! But yeah, it's the same as the one I've been looking at, so np:)

bvdmitri commented 2 years ago

@torfjelde Ahh, I see, I missed your 'EDIT' comment. Cool!

torfjelde commented 2 years ago

Btw, I opened an issue in the repo regarding the benchmark comparison to Turing.jl. It doesn't make the claims in the paper incorrect (the perf-diff is still an the order of magnitudes on conjugate models, just fewer orders), so no worries wrt. the paper submission :+1:

bvdmitri commented 2 years ago

Yes! I saw it and that is very useful! My laptop is in the office - I will definitely rerun these benchmarks tomorrow! I PM'ed you in Julia Slack regarding other models I use for benchmarking (they are not present in the main repository, but in my private one).

torfjelde commented 2 years ago

I've posted my full review above: https://github.com/JuliaCon/proceedings-review/issues/91#issuecomment-972963611.

Please let me know if there's something missing or you have any questions regarding what I've written :+1:

bvdmitri commented 2 years ago

Dear @torfjelde , first of all thank you for reviewing our work! We appreciate your effort and time you spent on it. I will just leave here a couple of remarks about your comments in https://github.com/JuliaCon/proceedings-review/issues/91#issuecomment-972963611 .

I do think we can improve both paper content and documentation a bit based on your comments.

all existing factor graph frameworks [...] use preset message sequence schedules

This is an indeed quite a statement without a context. You correctly assumed that we meant "As far as the authors are aware". We can definitely change it in our final version.

The comparison to a general-purpose PPL, e.g. Turing.jl, might give the impression that ReactiveMP.jl supports a larger family of models than it really does. Though it's good that the performance claim is limited to conjugate models:

Hm, that might be true. I'm not sure we explicitly outline somewhere in our documentation the scope of the package. This is due to the fact that ReactiveMP.jl is a successor of ForneyLab.jl. ForneyLab.jl does support inference in basically most of models that Turing.jl does with the help of nonlinear node and sampling. ReactiveMP.jl is supposed to be sort of ForneyLab.jl 2.0, but we didn't port and didn't reimplement all of the existing functionality from ForneyLab.jl. That is true that currently ReactiveMP.jl is limited to a specific family of models, namely conjugate state space models. More or less representative list of supported models (not all) can be found in demo/ folder (but you've probably already seen it).

EDIT: Ah, we do mention it in our README, but not in the documentation. I will change it as well.

I think we could be more explicit here! I can change it in README and repo documentation. What would you prefer? Would like me to outline it in the paper content as well? It is not a problem :)

Regarding performance claims, we test only models we can test ¯_(ツ)_/¯. Since in most of our models we use analytical solutions for messages and we do not use sampling I think we are guaranteed to be faster than sampling-based solutions (though it fallbacks again to the discussion that we do not support all models).

In general, any valid snippet of Julia code can be used inside the @model block.

Ah, I see now that this statement might be a bit confusing. Here we mean that you can use if statements, for statements, you can define lambda-functions in your model etc.. For example Soss.jl does not allow for blocks. We did not want to imply that inference will run without any errors in this statement. We may change this to be more clear.

unless one has defined nodes for each of the operations present at the top-level

You understood it correctly. Package consists of a set of standard nodes and message computations rules. We are constantly adding new nodes and rules, but it takes some time :) Currently if something does not work you either get a "missing node" error or "missing rule". It does not necessarily mean that package cannot run inference at all, but maybe we just missed some rules or still looking for a time to implement it.

Package is very young in that sense. We have another PhD project that is aimed to expand the scope of the package to more models and reduce number of such "missing something" errors. We also have plans to integrate with Turing.jl and Soss.jl at some point.

Finally, though I recognize that this would probably require quite a bit of effort/time: it would be really awesome to have comparisons to other software based on message-passing from outside the Julia community, e.g. Infer.NET.

That is true, I actually have comparison with Infer.Net in my private repos. The problem here is that I'm not an expert in C# at all. In addition, model specifications in C# are huge - they don't have very nice @macro lambdas :). I wrote a lot of code just for a setup and to make it look like in Julia and I'm not sure I did everything correct. Infer.NET gives reasonably performance btw (on models I managed to test), it is somewhere in between ReactiveMP and Turing.

Let me know what do you think and I will adjust the paper's content a bit. Thanks again for your time and comments!

torfjelde commented 2 years ago

This is an indeed quite a statement without a context. You correctly assumed that we meant "As far as the authors are aware". We can definitely change it in our final version.

Perfect!

Hm, that might be true. I'm not sure we explicitly outline somewhere in our documentation the scope of the package. This is due to the fact that ReactiveMP.jl is a successor of ForneyLab.jl. ForneyLab.jl does support inference in basically most of models that Turing.jl does with the help of nonlinear node and sampling. ReactiveMP.jl is supposed to be sort of ForneyLab.jl 2.0, but we didn't port and didn't reimplement all of the existing functionality from ForneyLab.jl. That is true that currently ReactiveMP.jl is limited to a specific family of models, namely conjugate state space models. More or less representative list of supported models (not all) can be found in demo/ folder (but you've probably already seen it).

EDIT: Ah, we do mention it in our README, but not in the documentation. I will change it as well.

I think we could be more explicit here! I can change it in README and repo documentation. What would you prefer? Would like me to outline it in the paper content as well? It is not a problem :)

Ah I missed the part in the README! And indeed, the statements in the README are exactly what I had in mind! And regarding the paper: as I mentioned above, just a brief sentence or even half a sentence about the restrictions ReactiveMP.jl would make the presentation even better. Something along the lines of "Even though ReactiveMP.jl does not currently have support for all models supported by more general-purpose PPLs, for the ones we do have support the performance is superior" (not exactly this statement, but you get the idea).

And because I feel the need to say this: I would say the exact same thing if Turing.jl wasn't even mentioned, e.g. if you were comparing to Stan instead. It's more just that for someone not super-familiar with the topic, reading the paper one could possibly walk away with the impression that "ReactiveMP.jl supports the same kind of models as Turing.jl/Stan, and ReactiveMP.jl is faster on conjugate models", and then a user installs ReactiveMP.jl and tries something rather trivial such as:

mymul(a, b) = a * b # or any user-defined function

@model function linear_gaussian_ssm_smoothing(n, A, B, P, Q)
    ...
    for t in 2:n
        x[t] ~ MvGaussianMeanCovariance(mymul(A, x[t - 1]), P)
        y[t] ~ MvGaussianMeanCovariance(mymul(B, x[t]), Q)    
    end

    return x, y
end

and then

model, vars = linear_gaussian_ssm_smoothing(n, A, B, P, Q);

results in

Unknown functional form 'mymul' used for node specification.

[...]

In contrast, in a general purpose PPL, e.g. Turing.jl or (AFAIK) Gen.jl, the above would just work.

But, as I said, I'm probably overly sensitive to this because of my previous experience interacting with the community; I've often found myself saying "Ehh for this problem Turing.jl is probably not the right choice; you want something more specialized." (and I'll be sure to recommend ReactiveMP.jl for models such as Linear SSM in high-perf-required settings going forward) or people just being generally quite confused as to why there are so many PPLs in Julia because we're not being clear enough regarding our pros and cons, and so I find myself gravitating towards being a bit clearer regarding this. (We can also definitively do a better job of this in Turing.jl!)

Regarding performance claims, we test only models we can test ¯(ツ)/¯. Since in most of our models we use analytical solutions for messages and we do not use sampling I think we are guaranteed to be faster than sampling-based solutions (though it fallbacks again to the discussion that we do not support all models).

I think you performance claims are completely fine! My previous comment was endorsing the fact that you explicitly mention you're comparing conjugacy models:)

Ah, I see now that this statement might be a bit confusing. Here we mean that you can use if statements, for statements, you can define lambda-functions in your model etc.. For example Soss.jl does not allow for blocks. We did not want to imply that inference will run without any errors in this statement. We may change this to be more clear.

Sounds good!

Package is very young in that sense. We have another PhD project that is aimed to expand the scope of the package to more models and reduce number of such "missing something" errors. We also have plans to integrate with Turing.jl and Soss.jl at some point.

As mentioned in DMs, I'm very much looking forward to this!

That is true, I actually have comparison with Infer.Net in my private repos. The problem here is that I'm not an expert in C# at all. In addition, model specifications in C# are huge - they don't have very nice @macro lambdas :). I wrote a lot of code just for a setup and to make it look like in Julia and I'm not sure I did everything correct. Infer.NET gives reasonably performance btw (on models I managed to test), it is somewhere in between ReactiveMP and Turing.

Yeah I figured this was probably the issue, hence why I said "though I realize this will be probably require a fair bit of effort". So this is completely fine!

bvdmitri commented 2 years ago

@whedon generate pdf from branch paper/julia_proceedings_2021

whedon commented 2 years ago 
Attempting PDF compilation from custom branch paper/julia_proceedings_2021. Reticulating splines etc...
whedon 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:

bvdmitri commented 2 years ago

@torfjelde I've update my paper based on your comments, you can quickly see changes here: commit1, commit2.

torfjelde commented 2 years ago

@bvdmitri Excellent! Really like the changes you made:)

bvdmitri commented 2 years ago

@matbesancon

Do we have any deadlines for JuliaCon2021 submission process? I would like to submit a full article about ReactiveMP (which goes very deep into details about reactive message passing and is very different from this submission) in the JMLR somewhere before January. They do have the following statement in their submission guide:

Submissions to JMLR cannot have been published previously in any other journal. We will consider submissions that have been published at workshops or conferences. In these cases, we expect the JMLR submission to cite the prior work, go into much greater depth and to extend the published results in a substantive way. ... We will also consider concurrent submissions of papers that are under review at conferences, provided the conference explicitly allows for this. ...

So I either need to cite the prior work (which is this paper) or I need an explicit permission from JuliaCon 2021 Proceedings for a cross-review.

ludgerpaehler commented 2 years ago

@matbesancon @bvdmitri

An excellent package, with exemplary documentation. In reviewing the individual checkpoints the only very minor "fault" I could find was with the README, and its equation rendering under GitHub's dark mode (which should be quite easy to fix I believe), i.e. see

https://github.com/biaslab/RxInfer.jl/issues/81

The paper is well written, but I would recommend the addition of a few articles at some points in the paper. Going through the points in the order of appearance in the paper:

In the abstract:

.. of variational family of distributions -> of the variational family of distributions

In section 2 Message Passing:

.. we exploit reactive programming approach -> we exploit the reactive programming approach..

In section 3 Reactive Message Passing:

We present ReactiveMP.jl package .. -> We present the ReactiveMP.jl package..

In our experiments new implementation scales comfortably ... -> In our experiments our/this new implementation scales comfortably..

, supports real-time data inference. -> , and support real-time data inference

Moreover, ReactiveMP.jl API supports various processing models... -> Moreover, ReactiveMP.jl's API supports various processing models..

In section 4 Conclusions (this is more of a style suggestion):

I would recommend to consider switching the "that" for a "which" in the following sentence.

We developed ReactiveMP.jl as a package that enables developers to builld ... -> We developed ReactiveMP.jl as a package which enables developers to build..

With the minor README rendering "issue" resolved and the non-style suggestions included, I would recommend direct acceptance to the proceedings.

bvdmitri commented 2 years ago

@ludgerpaehler thank you for your review! I adjusted text a bit based on your comments.

bvdmitri commented 2 years ago

@whedon generate pdf from branch paper/julia_proceedings_2021

whedon commented 2 years ago 
Attempting PDF compilation from custom branch paper/julia_proceedings_2021. Reticulating splines etc...
whedon 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:

ludgerpaehler commented 2 years ago

@matbesancon With the now included amendments, I would recommend direct acceptance.

matbesancon commented 2 years ago

@whedon check references

matbesancon commented 2 years ago

@whedon commands

whedon commented 2 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Re-invite a reviewer (if they can't update checklists)
@whedon re-invite @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# All commands can be run on a non-default branch, to do this pass a custom 
# branch name by following the command with `from branch custom-branch-name`.
# For example:

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to do a dry run of accepting the paper and depositing with Crossref
@whedon recommend-accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

EiC TASKS

# Invite an editor to edit a submission (sending them an email)
@whedon invite @editor as editor

# Reject a paper
@whedon reject

# Withdraw a paper
@whedon withdraw

# Ask Whedon to actually accept the paper and deposit with Crossref
@whedon accept deposit=true
matbesancon commented 2 years ago

@whedon check references from from branch paper/julia_proceedings_2021

matbesancon commented 2 years ago

@whedon check references from branch paper/julia_proceedings_2021

whedon commented 2 years ago 
Attempting to check references... from custom branch paper/julia_proceedings_2021
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1137/141000671 is OK
- 10.3390/e23070807 is OK
- 10.1287/opre.38.5.911 is OK
- 10.1016/j.ijar.2018.11.002 is OK

MISSING DOIs

- None

INVALID DOIs

- None
matbesancon commented 2 years ago

@whedon recommend-accept from branch paper/julia_proceedings_2021

whedon commented 2 years ago

No archive DOI set. Exiting...

matbesancon commented 2 years ago

@bvdmitri can you archive the repo on zenodo, precisely that corresponds to the version used in the paper

bvdmitri commented 2 years ago

@matbesancon We archived our repo on Zenodo here https://zenodo.org/record/5913617 . All statements in the paper are valid for the latest release too (we even improved our performance better and it became faster since the last JuliaCon).

matbesancon commented 2 years ago

@whedon set 10.5281/zenodo.5913617 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5913617 is the archive.

matbesancon commented 2 years ago

@whedon set v1.3.1 as version

whedon commented 2 years ago

OK. v1.3.1 is the version.