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

Allow MeanField constraints #189

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

wouterwln
Copy link
Member

This PR allows the following syntax:

@constraints begin
    q(x, y) = MeanFieldI()
    q(z) = MeanField()
end

While building this, I also found a bug. When mat is a matrix valued RV and the user specified:

@constraints begin
    q(mat) = q(mat[begin])..q(mat[end])
end

and obscure error was thrown. Because of the way we encode ranges in the current implementation we actually don't allow multidimensional splitted ranges so a helpful error message will now be thrown.

@wouterwln wouterwln requested a review from bvdmitri March 20, 2024 11:44
Copy link
Member

@bvdmitri bvdmitri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is at least

q(mat) = MeanField()

allowed?

@wouterwln
Copy link
Member Author

@bvdmitri of course it is, it goes through a different channel since MeanField is treated differently

…gine.jl

Co-authored-by: Bagaev Dmitry <bvdmitri@gmail.com>
@bvdmitri
Copy link
Member

Good! Can we add a testcase where we apply MeanField for a matrix-based or even higher dimensional variables just in case? Such that we don't break it accidentally, doesn't need to be a valid model, just usage of matrix-based variables.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.05%. Comparing base (1ded935) to head (8b6b0ac).
Report is 10 commits behind head on dev-4.0.0.

Files Patch % Lines
...onal_constraints/variational_constraints_engine.jl 92.85% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           dev-4.0.0     #189      +/-   ##
=============================================
+ Coverage      91.92%   92.05%   +0.13%     
=============================================
  Files             15       15              
  Lines           1845     1863      +18     
=============================================
+ Hits            1696     1715      +19     
+ Misses           149      148       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wouterwln
Copy link
Member Author

Added a test that tests matrices

@wouterwln wouterwln requested a review from bvdmitri March 20, 2024 12:25
Copy link
Member

@bvdmitri bvdmitri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@wouterwln wouterwln merged commit 5c10550 into dev-4.0.0 Mar 20, 2024
8 checks passed
@wouterwln wouterwln deleted the 188-meanfield-constraint-in-constraints-macro branch March 20, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants