Made sample_shape same across all contexts in draw_values

[Sayam753]

Thank your for opening a PR!

Depending on what your PR does, here are a few things you might want to address in the description:

• what are the (breaking) changes that this PR makes?
  Closes Issue with prior_predictive_sample and MvNormal #3758
• important background, or details about the implementation

There was a minor issue in this condition with respect to size.
https://github.com/pymc-devs/pymc3/blob/de47253ce901ac9afac3a0fe33d3cb96f7f354db/pymc3/distributions/distribution.py#L703-L704

Coming to the code,

ndim = 50
with pm.Model() as model:
    a = pm.Normal("a", sd=100, shape=ndim)
    c = pm.MvNormal("c", mu=a, chol=np.linalg.cholesky(np.eye(ndim)), shape=ndim)
    samples = pm.sample_prior_predictive(1000)

Variable a is drawn as ('a ~ Normal', 1000) and stored in drawn_vars. But when looking for MvNormal, the condition is evaluated as ('a ~ Normal', (1000, )) with size as tuple and there is a mismatch. So, I converted the size beforehand using to_tuple to ensure a common base sample_shape across contexts.

Another approach could have been the use to_tuple before passing size to draw_values. But this needs to be done for all distributions. So, I opted for the first approach.

• are the changes—especially new features—covered by tests and docstrings?
  Yes. Earlier in Fix MvNormal.random #4207, I have written a test for first half of the issue Issue with prior_predictive_sample and MvNormal #3758. This PR just appends that test with the second half.
• right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

[codecov]

Codecov Report

Merging #4305 (da81fd8) into master (4fd56fd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4305      +/-   ##
==========================================
- Coverage   87.56%   87.54%   -0.02%     
==========================================
  Files          88       88              
  Lines       14270    14272       +2     
==========================================
  Hits        12495    12495              
- Misses       1775     1777       +2     

Impacted Files	Coverage Δ
pymc3/distributions/distribution.py	95.03% <100.00%> (+0.75%)	⬆️
pymc3/distributions/simulator.py	82.43% <100.00%> (+0.24%)	⬆️
pymc3/distributions/multivariate.py	82.75% <0.00%> (-0.58%)	⬇️
pymc3/distributions/posterior_predictive.py	89.01% <0.00%> (-0.29%)	⬇️

[Sayam753]

I handled size this way because now it is passed as a tuple. Ping @aloctavodia to review as this is related to SMC stuff.
Also, test suite passes. :)

[Sayam753]

I rebased to current master. So, the PR is ready for review.

[twiecki]

Suggested change

	if not size:
	if len(size) != 0:

[Sayam753]

Oh, I am checking for size being empty tuple here. So, do you mean len(size) == 0?

[twiecki]

Yes.

[Sayam753]

But if there is a call to this random method directly, then size will be None. I prefer to leave it as is, since it handles both cases (being None or empty tuple).

[twiecki]

Relying on the truthiness of an object can lead to really subtle and difficult to debug bugs. In that case I would be explicit, even if it doesn't look as clean:
if (size is None) or (len(size) == 0):

[MarcoGorelli]

can lead to really subtle and difficult to debug bugs

Indeed

[Sayam753]

Yes. I like this idea. Now, I have opted to use to_tuple to better handle corner cases.

[twiecki]

@Sayam753 Looks great, can you add this to the release-notes?

[twiecki]

Thanks @Sayam753!