ReactiveMP.jl icon indicating copy to clipboard operation
ReactiveMP.jl copied to clipboard

Add CVI implementation for the delta node

Open Nimrais opened this issue 2 years ago • 4 comments

Nimrais avatar Aug 12 '22 08:08 Nimrais

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Codecov Report

Base: 76.72% // Head: 76.85% // Increases project coverage by +0.12% :tada:

Coverage data is based on head (249dd57) compared to base (ee71a88). Patch coverage: 72.40% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   76.72%   76.85%   +0.12%     
==========================================
  Files         203      211       +8     
  Lines        6723     7175     +452     
==========================================
+ Hits         5158     5514     +356     
- Misses       1565     1661      +96     
Impacted Files Coverage Δ
src/distributions/gamma_shape_rate.jl 94.11% <ø> (ø)
src/message.jl 70.88% <0.00%> (-0.55%) :arrow_down:
src/nodes/delta/layouts.jl 0.00% <0.00%> (ø)
src/nodes/delta/marginals.jl 0.00% <0.00%> (ø)
src/nodes/gamma_mixture.jl 3.88% <0.00%> (-0.42%) :arrow_down:
src/rules/gamma_mixture/switch.jl 0.00% <ø> (ø)
src/rules/gamma_shape_rate/a.jl 0.00% <0.00%> (ø)
src/rules/normal_mixture/out.jl 100.00% <ø> (ø)
src/rules/normal_mixture/switch.jl 66.66% <ø> (ø)
src/marginal.jl 59.09% <33.33%> (-1.23%) :arrow_down:
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Aug 15 '22 12:08 codecov-commenter

what's the progress status of this PR?

albertpod avatar Aug 19 '22 11:08 albertpod

Hey @Nimrais @albertpod ! Thanks for working on this PR!

I've just started reviewing this PR, but I have some comments already. I would like them to be addressed before further reviewing. I asked many times during our update meetings to cleanup the demo/ folder and ensure that everything works nice and smooth and that there are no broken demos, however:

  • Most of the demo notebooks haven't been touched or updated in weeks / months
  • Eight school cvi notebook does not work and does not have any explanation in it
  • multiple_interfaces_cvi notebook does not work and does not have any explanation in it
  • Nonlinear observation of Dynamical System does not work
  • Mimic control flow trough delta node notebook does not work and does not have explanation in it
  • Generic nonlinear delta node notebook served as a small starting point and must be removed (and it does not run either)
  • ℹ️ fl_smoothing the only notebook that does execute, but has no explanation and has some debug cells in it
  • fl_smoothing_fixed_R_W does not work and does not have any explanation in it
  • fl_* has been used to indicate that a notebook has been written in ForneyLab.jl, I asked to remove this prefix
  • I asked to move demos out of the sensor_fusion_cvi folder and put the data in to the data folder instead
  • Notebooks must not extend the functionality of the ReactiveMP (adding new rules or methods to the ReactiveMP), unless its the purpose of the notebook
  • This branch has duplicated the old Normalizing flow notebook tutorial, on the master it has been renamed to the Invertible Neural Network Tutorial
  • In addition to the previous one, we discussed that it would be nice to have something like CVI tutorial, I had an impression that it is ready, but I couldn't find it. In any case, in order to merge this PR we need either a tutorial notebook or a documentation page (both is perfect) explaining how to use CVI
  • Tests are failing locally (SampleList related, master branch should be fine, probably a simple merge would fix this issue)

I do not really mind that something might be broken at this stage, its totally fine. But I do mind that on our last update meeting I've been told that everything has been fixed, cleaned-up and all demo work. Now I see (in the commit history at least) that all of the demo notebooks haven't been touched in weeks and some haven't been updated in months. I would like to ask @Nimrais in the future, before you mark any PR as ready for review, please make sure it is ready. It took quite some time to execute all notebooks and to ensure and double-check that the problem is not with the configured environment, packages installation, my Julia version etc, but with the state of the PR itself.

Please, ping me when the issues outlined above are resolved, such that I can continue the review.

bvdmitri avatar Oct 14 '22 13:10 bvdmitri