Remove shape functions #6556
Remove shape functions #6556
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6556 +/- ##
===========================================
+ Coverage 41.29% 89.33% +48.03%
===========================================
Files 93 93
Lines 15717 15684 -33
===========================================
+ Hits 6491 14011 +7520
+ Misses 9226 1673 -7553
|
pymc/distributions/multivariate.py
Outdated
| @@ -1651,7 +1650,9 @@ def rng_fn(cls, rng, mu, rowchol, colchol, size=None): | |||
| output_shape = size + dist_shape | |||
|
|
|||
| # Broadcasting all parameters | |||
| (mu,) = broadcast_dist_samples_to(to_shape=output_shape, samples=[mu], size=size) | |||
| shapes = [mu.shape, output_shape] | |||
| sp_shapes = [s[len(size) :] if size == s[: min([len(size), len(s)])] else s for s in shapes] | |||
There was a problem hiding this comment.
Did you test this by hand?
I'd feel better it this line was covered by tests, or refactored to be easier to comprehend (or both)
There was a problem hiding this comment.
To test that we are getting the correct broadcastable shape, I think the best option is to keep the function broadcast_dist_samples_shape and its tests. shapes_broadcasting is replaced by np.broadcast_shapes. PR updated.
Also, more tests may be needed, using only what I added here was not correct but all tests were passing.
|
You may be able to use this utility to broadcast everything: https://github.com/pymc-devs/pytensor/blob/63f8d6e732cd19e7f8940e25e4c44231e7765f96/pytensor/tensor/random/utils.py#L24 Although I think this RV was never properly implemented as a RandomVariable. For instance if |
|
This PR may need a rebase before we can merge |
I haven't had the time to review this ☝🏽 |
|
We can rebase and merge this one as is if it's working and keep cleaning it up in future PRs |
symeneses
force-pushed
the
remove-shape-functions
branch
from
c13206d to
c937001
Compare
|
Opened an issue for the MatrixNormal, #6606 which I am not very confident is (or was) working correctly. Would have to check what actual tests there are. Otherwise the changes seem sensible. Thanks @symeneses |
#6066 is a PR not an issue. I could have a look to the tests based on your previous comment and in the new issue. |
|
Thanks, it was a typo. The issue is #6606 |
What is this PR about?
It solves #6107
Checklist
Major / Breaking Changes
New features
Bugfixes
Documentation
Maintenance