Refactored Wishart and MatrixNormal distribution #4777
Conversation
| sizes_to_check = [None, (), 1, (1,), 5, (4, 5), (2, 4, 2)] | ||
| sizes_expected = [ | ||
| (3, 3), | ||
| (3, 3), | ||
| (1, 3, 3), | ||
| (1, 3, 3), | ||
| (5, 3, 3), | ||
| (4, 5, 3, 3), | ||
| (2, 4, 2, 3, 3), | ||
| ] |
There was a problem hiding this comment.
What extra logic (e.g. implemented in WishartRV) are all these sizes testing?
Remember, we don't want the test time to grow larger without proportionally increasing the actual test coverage (and that coverage doesn't include NumPy/SciPy's code). This test module is easily one of the most egregious violators of that principal, so we should always be a little more critical of additions to it.
There was a problem hiding this comment.
Should that be checking only for edge cases like None and () ? I just took the sizes_to_check list from the original function in BaseTestDistribution where the sizes are being checked by default. (And constructed the corresponding expected list.)
There was a problem hiding this comment.
To echo @kc611, this is not actually testing more than what the default test does, just setting the specific parameters of the size test, which usually have to be adapted for multivariate distributions.
There was a problem hiding this comment.
I agree to both: This implementations is very much in line with the defaults.
The the BaseTestDistribution for sure takes a sledgehammer to break a nut, but that problem is out of scope of this PR.
There was a problem hiding this comment.
Pushing admittedly poor defaults forward is a great way to get into a bad situation, and an even better way to get deeper into existing ones.
Also, the answer is not one big PR that removes all the testing redundancies; given the knowledge and effort required for that, it's quite unrealistic. Plus, that won't prevent the problem from reappearing again and again after such a heroic PR.
At the very least, we need to stop adding to the testing redundancies on a case-by-case basis, and we can do that by looking for it during reviews; otherwise, we're just hoping that contributors will do that for us, and that's not how things have turned out.
Regarding these exact tests, if they're only targeting WishartRV.rng_fn, it looks like we only need a test for an empty and non-empty size (and perhaps one other), because that's the only relevant condition in the code added by this PR. Every other value besides the first two appears to be testing the code in RandomVariable and scipy.stats.wishart.rvs.
There was a problem hiding this comment.
I think there's no harm in not testing the [ (), (1,), 5, (2, 4, 2)] cases if we are testing for [None, 1, (4, 5)] cases since they're pretty much the same thing with different values. (Updated accordingly)
kc611
force-pushed
the
add_wish
branch
2 times, most recently
from
cd4c3c6
to
fe07893
Compare
There was a problem hiding this comment.
@kc611 take a look at my comments and tell us when you see this as ready to merge :)
| sizes_to_check = [None, (), 1, (1,), 5, (4, 5), (2, 4, 2)] | ||
| sizes_expected = [ | ||
| (3, 3), | ||
| (3, 3), | ||
| (1, 3, 3), | ||
| (1, 3, 3), | ||
| (5, 3, 3), | ||
| (4, 5, 3, 3), | ||
| (2, 4, 2, 3, 3), | ||
| ] |
There was a problem hiding this comment.
I agree to both: This implementations is very much in line with the defaults.
The the BaseTestDistribution for sure takes a sledgehammer to break a nut, but that problem is out of scope of this PR.
kc611
force-pushed
the
add_wish
branch
2 times, most recently
from
65b49fc
to
80879e7
Compare
This PR refactors the
WishartandMatrixNormalmultivariate distributions.