PointMassFormConstraint exception for DirichletCollection + test #529
Conversation
🤖 Code FormattingYour PR still has some code formatting issues. I've updated PR #530 with the necessary formatting changes. You can merge that PR into this branch to fix the code style check. Alternatively, you can run |
|
Good job! I've left some comments on the way we'd like to see the tests structured, but as a first draft this looks good! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #529 +/- ##
=======================================
Coverage 80.11% 80.12%
=======================================
Files 25 25
Lines 2012 2013 +1
=======================================
+ Hits 1612 1613 +1
Misses 400 400 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I'm a bit confused on what this PR is doing. Could you clarify what are you trying to achieve here? That would help me to guide you.
Your PR adds a method inside of the tests, which is not allowed in a package. Also it doesn't test anything. Could you please update the PR such that all the required methods are added in the package and that you have at least one @test or @test_throws inside the @testset
| end | ||
| end | ||
|
|
||
| Distributions.mode(d::DirichletCollection) = error("`mode` should not be called on DirichletCollection.") |
There was a problem hiding this comment.
You should not add new methods inside the tests
| Distributions.mode(d::DirichletCollection) = error("`mode` should not be called on DirichletCollection.") |
There was a problem hiding this comment.
The reason for that is that tests are not being executed when using the package, tests are only executed by developers and not users. Thus if you attempt to add a new functionality inside tests the users will not actually get this functionality because they don't run tests
| @testset "`DirichletCollection` exception (mode is not defined)" begin | ||
| constraint = PointMassFormConstraint() | ||
| d = DirichletCollection(ones(3, 3)) | ||
| opt = constrain_form(constraint, d) |
There was a problem hiding this comment.
Can you add a @test that tests the output?
There was a problem hiding this comment.
Or if you want to test that this operation actually throws an error use @test_throws
There was a problem hiding this comment.
@ThijsJenneskens you can read about the differences here https://docs.julialang.org/en/v1/stdlib/Test/
| ReactiveMP.constrain_form(::PointMassFormConstraint, distribution::Distribution) = PointMass(mode(distribution)) | ||
|
|
||
| # Except for the DirichletCollection. For this one, we will use the mean instead: | ||
| ReactiveMP.constrain_form(::PointMassFormConstraint, distribution::DirichletCollection) = PointMass(mean(distribution)) |
There was a problem hiding this comment.
I'm a bit confused, so why do we need this method if we test for exception such that the mode is not defined? The method above calls the mode but this one calls the mean?
There was a problem hiding this comment.
We need this method because the mode is not defined for the Dirichlet Collection. I added the test such that, if a future change does end up calling the mode on a Dirichlet Collection random variable somehow, it's clear what goes wrong.
|
If you want to add a method for the |
As in, use multiple dispatch to redefine the mode of a DirichletCollection to it's mean? That could be a better solution. However, maybe we want users to receive a warning if they try to call mode on a Dirichlet Collection? |
|
@ThijsJenneskens you can implement |
3 participants
Fix issue #343 by adding an exception for the DirichletCollection. Also added a test to check that mode is not being called on a DirichletCollection random variable.