feat: add @assert length check into setmarginals! and setmessages! #366
Conversation
Nimrais
changed the title
src/variables/variable.jl
Outdated
| struct RepeatedValueContainer | ||
| marginal | ||
| container | ||
| _length | ||
| end | ||
|
|
||
| Base.length(ri::RepeatedValueContainer) = ri._length | ||
| Base.iterate(ri::RepeatedValueContainer) = iterate(ri.container) | ||
| Base.iterate(ri::RepeatedValueContainer, state) = iterate(ri.container, state) | ||
|
|
||
| function RepeatedValueContainer(marginal, _length) | ||
| container = Iterators.repeated(marginal, _length) | ||
| return RepeatedValueContainer(marginal, container, _length) | ||
| end | ||
|
|
There was a problem hiding this comment.
| struct RepeatedValueContainer | |
| marginal | |
| container | |
| _length | |
| end | |
| Base.length(ri::RepeatedValueContainer) = ri._length | |
| Base.iterate(ri::RepeatedValueContainer) = iterate(ri.container) | |
| Base.iterate(ri::RepeatedValueContainer, state) = iterate(ri.container, state) | |
| function RepeatedValueContainer(marginal, _length) | |
| container = Iterators.repeated(marginal, _length) | |
| return RepeatedValueContainer(marginal, container, _length) | |
| end |
I don't understand why is this needed? (+ it is slower than just using Iterators.repeated because all types are Any). Lets not create duplicate functionality with the Base.
There was a problem hiding this comment.
@bvdmitri you are right It's not needed, my reasoning about iterators length function was wrong. I checked the docs.
test/variables/test_variable.jl
Outdated
| @@ -61,6 +62,7 @@ using Test, ReactiveMP, Rocket, BayesBase, Distributions, ExponentialFamily | |||
| activate!.(variablesmx, TestOptions()) | |||
|
|
|||
| setmarginals!(variablesmx, dist) | |||
| @test_throws AssertionError setmarginals!(variablesmx, [dist]) | |||
There was a problem hiding this comment.
| @test_throws AssertionError setmarginals!(variablesmx, [dist]) | |
| @test_throws AssertionError setmarginals!(variablesmx, dist[begin:end-1]) |
There was a problem hiding this comment.
I missed this one. What do you mean?
There was a problem hiding this comment.
@Nimrais
I just noticed that the purpose of this test might be a bit unclear (in the future), as it currently seems to throw errors for two reasons rather than solely due to a mismatched length. Specifically, since dist is a matrix and [dist] is an array of matrices, this combination isn't supported and may also throw. To effectively test the length, you can exclude the last element from dist. A straightforward way to achieve this is by using dist[begin:end-1], which will include all elements of dist except the last one and ensure that the length is equal to N - 1.
There was a problem hiding this comment.
We do not have any multivariate distributions in this test, only these ones: NormalMeanVariance(-2.0, 3.0), NormalMeanPrecision(-2.0, 3.0), PointMass(2.0).
There was a problem hiding this comment.
Ah sorry, you are right, I confused myself with the variablesmx variable. The updated tests are also good!
There was a problem hiding this comment.
But I will add it now, your concern is reasonable.
There was a problem hiding this comment.
As I can see you've added it here already (and this is exactly what I've ment in my previous messages). This is enough :)
Co-authored-by: Bagaev Dmitry <bvdmitri@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 60.20% 61.24% +1.04%
==========================================
Files 174 174
Lines 5769 5801 +32
==========================================
+ Hits 3473 3553 +80
+ Misses 2296 2248 -48 ☔ View full report in Codecov by Sentry. |
This PR aims to resolve ReactiveBayes/RxInfer.jl#175.