-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Added check that nu must be a scalar in MvStudentTRV #5241
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5241 +/- ##
=======================================
Coverage 79.12% 79.13%
=======================================
Files 88 88
Lines 14306 14311 +5
=======================================
+ Hits 11320 11325 +5
Misses 2986 2986
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I left a suggestion below, and we should add a test that covers this branch.
Perhaps in test_distributions_random
you can add something along the lines of the following one:
pymc/pymc/tests/test_distributions_random.py
Lines 1622 to 1632 in 95bd5e5
def test_errors(self): | |
msg = "MatrixNormal doesn't support size argument" | |
with pm.Model(): | |
with pytest.raises(NotImplementedError, match=msg): | |
matrixnormal = pm.MatrixNormal( | |
"matnormal", | |
mu=np.random.random((3, 3)), | |
rowcov=np.eye(3), | |
colcov=np.eye(3), | |
size=15, | |
) |
2ca0fca
to
24316b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I like the re.escape(msg)
it always takes me ages to find the right escape syntax..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
This should fix issue #5214