Skip to content

Conversation

@ricardoV94
Copy link
Member

The default method is not robust and can lead to cryptic error messages.

In a discussion with a user https://discourse.pymc.io/t/pm-sample-prior-predictive-fails-with-incorrect-size-info-passed-to-the-rng-fn-function-of-the-randomvariable/12513 , they were trying to fake a multivariate distribution as a univariate because of the cryptic error induced by the default method.

We now raise NotImplementedError by default, and include illustrative examples in the docstrings that should help users implementing this method.

Also fixed a couple of minor bugs.

The changes will break RVs that relied on the default implementation. This should be easy to fix and won't go unnoticed.

The errors raised by the default when it fails are rather cryptic

Also fix bug in helper function
The RV always expects a vector input and `ndims_paramas` is always `[1]`.

Size is no longer ignored
@codecov-commenter
Copy link

Codecov Report

Merging #388 (c689549) into main (9be43d0) will decrease coverage by 0.01%.
The diff coverage is 97.82%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #388      +/-   ##
==========================================
- Coverage   80.46%   80.46%   -0.01%     
==========================================
  Files         156      156              
  Lines       45514    45517       +3     
  Branches    11148    11148              
==========================================
+ Hits        36624    36626       +2     
- Misses       6688     6689       +1     
  Partials     2202     2202              
Impacted Files Coverage Δ
pytensor/tensor/random/op.py 96.62% <93.75%> (-0.87%) ⬇️
pytensor/tensor/random/basic.py 99.04% <100.00%> (+<0.01%) ⬆️
pytensor/tensor/random/utils.py 100.00% <100.00%> (ø)

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ricardoV94 ricardoV94 merged commit 673c1ac into pymc-devs:main Jul 22, 2023
@ricardoV94 ricardoV94 deleted the mv_rv_default branch July 22, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants