Remove Dirichlet distribution type restrictions

[brandonwillard]

The changes in this PR allow the Dirichlet distribution to take Theano types for the distribution argument.

The current implementation of Dirichlet only effectively allows NumPy types for its distribution arguments, and, since the tests are all performed on NumPy input, this important case was overlooked.

Also, because of the aforementioned testing situation, it was possible to use Dirichlet without having to specify the shape keyword; however, this apparent "feature" was only a side-effect of the incorrect assumption that all Dirichlet parameters should be NumPy arrays, instead shape values should always be required—as they are for most/all other multivariate distributions.

Closes #3999.

[brandonwillard]

While setting this up, I noticed that our testing framework—TestMatchesScipy in particular—doesn't appear to perform tests using Theano arguments. This seems like a pretty big oversight, no?

[codecov]

Codecov Report

Merging #4000 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4000   +/-   ##
=======================================
  Coverage   86.77%   86.77%           
=======================================
  Files          88       88           
  Lines       14137    14134    -3     
=======================================
- Hits        12267    12265    -2     
+ Misses       1870     1869    -1     

Impacted Files	Coverage Δ
pymc3/distributions/multivariate.py	78.17% <100.00%> (+0.04%)	⬆️

[lucianopaz]

Everything looks good except for what @junpenglao noted about shape. With this change, although it will make the Dirichlet be consistent with the rest of the pymc3 distributions, we'll break existing code.

[brandonwillard]

Nevertheless, this change will break existing code that doesn't specify a shape (just look at the amount of tests you had to fix). We'll have to add a patch that tries to get the static shape from the parameters and pass it to the distribution kwargs.

While true, the "greater" truth is that it is arguably more broken in its current state. The existing code that will break due to these changes is also operating in err, and the expectations it sets are themselves confusing and error-prone (e.g. we don't have to specify shape here, but most everywhere else?). I don't see a path down which we can continue to allow this user-level use of Dirichlet and it leads to anything good—without also solving the shape problems, that is.

[AlexAndorra]

Where do we stand on this? It seems like there is no way to make Dirichlet consistent with the rest of PyMC3 distributions without making a breaking change -- do you agree @lucianopaz ?

[lucianopaz]

I think that it is possible. There are two ways to do it:

1. A quick and dirty patch in the dirichlet distribution itself to make it infer the shape when the concentration isn't a theano related instance. From the prior discussion, Brandon is against this.
2. Write something more general at the Distribution base class level that can infer a shape from the parameters when they aren't theano things. This would require some major work to include a bunch of meta information in each distribution so that it knows what are the distribution's parameters and how to infer a shape from them. Again, from what we discussed so far, @brandonwillard prefers this, but it goes way beyond this particular PR.

I like option 2 the most, but it will take a lot of work. I think that we should write a dirty patch, as in option 1, so we can merge this PR, and then address option 2 with a different dedicated PR.

[AlexAndorra]

I like that too (and option 1 is kind of already what we were doing before this PR, IIUC), although the difficulty could be to find volunteers to implement option 2 once this PR is merged 😬

[brandonwillard]

I've added a test value shape inference fallback accompanied by a deprecation warning.

Again, this sort of functionality is irredeemably wrong, since the shapes inferred from test values can very easily be incorrect when shared variables are involved, or when the relevant Theano graphs are compiled and called with inputs that differ from the test values.

[brandonwillard]

Looks like we have some errors due to Matplotlib version differences.

See #4022.

[lucianopaz]

Everything seems fine now. Thanks @brandonwillard!