Fixing scalar shape handling: treat (1,) as vectors

[michaelosthege]

Breaking Changes

• From now on () is the only scalar shape. (1,) will become vector of length 1

New Shape Examples

• Normal("n", shape=None) is the same as the default Normal("n"). Here it is up to the distribution (most will default to ())
• shape=() explicitly specifies scalar shape
• shape=0 or shape=(0,) is up for discussion (see below)
• shape=1 is the same as shape=(1,) is a length 1 vector

Open ToDos

• This blows up hundreds of a few tests. They must be fixed.
• Make sure to raise a VelueError when a user passes shape=(0,) / shape=0 to a distribution
• size=(0,)/size=0 to .random() remains allowed - because numpy also allows it on the random number generator
• Check out the RandomWalk distributions. While the tests cover shapes, they don't necessarily catch problems like GaussianRandomWalk prior predictive is broken #3962 (they look fine, but most don't have a .random() implementation. See .random() methods missing from some timeseries distributions #4337 )
• release notes
• re-running relevant notebooks (oh dear!) Let's do this on a separate PR

[AlexAndorra]

Thanks for taking the initiative @michaelosthege !
First comment: "re-running relevant notebooks (oh dear!)" --> 🤣

[michaelosthege]

I think I ran into the same problem as @StephenHogg in #4211

EDIT: I removed the traceback, because it's now documented in #4219

I'm on a quite recent Theano-PyMC master branch (4249a0). (Maybe you are too, @StephenHogg?)

@brandonwillard assuming that this is indeed due to a change in Theano-PyMC, do you know about this issue already? Any hints?

[codecov]

Codecov Report

Merging #4214 (0a5b04b) into master (34447a7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4214      +/-   ##
==========================================
+ Coverage   87.95%   87.97%   +0.02%     
==========================================
  Files          88       88              
  Lines       14499    14483      -16     
==========================================
- Hits        12752    12741      -11     
+ Misses       1747     1742       -5     

Impacted Files	Coverage Δ
pymc3/distributions/mixture.py	88.69% <ø> (+0.26%)	⬆️
pymc3/distributions/posterior_predictive.py	89.23% <ø> (-0.07%)	⬇️
pymc3/distributions/discrete.py	95.82% <100.00%> (+0.59%)	⬆️
pymc3/distributions/distribution.py	95.40% <100.00%> (+0.09%)	⬆️
pymc3/distributions/multivariate.py	82.90% <0.00%> (+0.14%)	⬆️
pymc3/math.py	68.64% <0.00%> (+0.17%)	⬆️

[StephenHogg]

As far as theano-pymc goes, here's what pip freeze has to say:

Theano==1.0.5
Theano-PyMC==1.0.10

The error looks the same, yes.

[brandonwillard]

The key line is here:

pymc3\model.py:1800: in __init__
    self.tag.test_value = theano.compile.view_op(data).tag.test_value

We recently introduced a change to Theano's test value handling that validates the test values up front. The fix requires that the PyMC3-assigned test values be cast to the appropriate types (e.g. theano.compile.view_op(data).tag.test_value needs to be changed to something like theano.compile.view_op(data).tag.test_value.astype(self.dtype)).

[lucianopaz]

All changes here look fine.

However, i know that the MixtureSameFamily also does a check for size=1 that should be removed here. I imagine that other "meta distributions" like the Mixture and DensityDist do the something similar. Could you check those too?

[michaelosthege]

@lucianopaz thanks. Only in MixtureSameFamily I found the corresponding lines.

[twiecki]

You know it's a good PR when:

[twiecki]

Thanks!

[mjhajharia]

can I work on this?

[ricardoV94]

can I work on this?

Did you mean this: #3896

[mjhajharia]

yep

…

On 17 Mar 2021, 8:14 PM +0530, ricardoV94 ***@***.***>, wrote: > can I work on this? Did you mean this: #3896 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.