Extend unittests for some distributions.

[zoj613]

This commit fixes/completes/extends previously minimal unittests for
some of the available probability distribution samplers, which include:

• multivariate_normal_cong2017
• multivariate_normal_bhattacharya2016.

This is a followup of #32 and closes #33

[codecov]

Codecov Report

Merging #42 (97f907c) into main (7e888a0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #42   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          295       288    -7     
  Branches        20        20           
=========================================
- Hits           295       288    -7     

Impacted Files	Coverage Δ
aemcmc/dists.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e888a0...97f907c. Read the comment docs.

[zoj613]

For what its worth: the multivariate_normal_bhattacharya2016 function is a special case of multivariate_normal_cong2017 for when the omega parameter is the identity matrix, so it can be written as:

def multivariate_normal_bhattacharya2016(rng, D, phi, alpha):
    return multivariate_normal_cong2017(rng, 1 / D, at.ones(phi.shape[0]), phi, alpha)

So not sure if this is worth a refactor and that the overhead of creating the at.ones and the extra steps inside the multivariate_normal_cong2017 function wont affect performance.

Cc @rlouf @brandonwillard

[zoj613]

BTW @rlouf while working on this PR today I realized that the tests in #32 previously failed for two reasons:

1. you were using passing phi as a transpose of the expected matrix in the cong2017 function. Also the phi need for the other one should be computed from the cholesky since omega is the identity (so the eigen decomposition was not going to work since it produces a diagonal matrix that isnt the identity).
2. the true variance should have been the diagonal entries of the covariance matrix of the input data so that the samples from the scan function can be correctly compared to those entries.

Which leads me to think that the dist.py module should get another PR (or extra commits on this one) that elaborate a bit more on the documentation about the expected input and how it could be generated, as well as some typehints.

[rlouf]

Thank you! I have not seen multivariate_normal_bhattacharya2016 used anywhere. If so we should remove it altogether.

For the documentation it was not always easy to find the corresponding parametrization in the references. A short explanation of the meaning of each parameter and how they relate to mean/covariance would be very useful.

Besides these two point this looks good to me.

[zoj613]

Thank you! I have not seen multivariate_normal_bhattacharya2016 used anywhere. If so we should remove it altogether.

For the documentation it was not always easy to find the corresponding parametrization in the references. A short explanation of the meaning of each parameter and how they relate to mean/covariance would be very useful.

Besides these two point this looks good to me.

Do you feel these two points should be a separate PR or part of this one?

[rlouf]

They can be part of this one; it will take you more time to open a separate PR than make these changes.

[zoj613]

@rlouf i've addressed your last 2 points in the latest commit. Please take a look at this again when you can,.

[rlouf]

The changes look good to me, however you will need to make some changes to your commits' structure:

1. No need for TST/DOC, etc in the commit title
2. Can you break down you last commit in two: removing one function + adding some documentation? These are two separate changes.

[rlouf]

Looks good to me.

[rlouf]

Nitpick: docstring should start after the """

[zoj613]

I think this is mainly a preference thing. The current way is still correct according the numpydoc convention and is used all over numpy and scipy source code. The pydocstyle linter doesn't complain either on my editor.

[rlouf]

When you start a repo every reasonable choice is certainly a question of preference, in an existing codebase it is a matter of consistency. This format is used in other aesara-related projects and I don't see a reason to do something different here.

[zoj613]

I understand your concern but dists.py is the first module commited into the codebase and if you look at the other functions in it, they use the same docstring convention. That is about as consistent as can be in this case. Using the suggested preference would introduce an inconsistent docstring style in the module. So the point made here is a bit contradictory.

[rlouf]

There are only two consistent choices here: change the docstrings in every other files in the repository, or press DEL twice in this file. Whatever you think is best.

[brandonwillard]

All the aesara-devs projects follow the code and documentation standards of Aesara. In the case of docstrings, we use NumPy's format—more or less. (The link in our documentation is broken; it should be this.)

We have some legacy docstrings in the Aesara repo that go against this standard, but those need to be fixed.

[brandonwillard]

FYI: the aesara-devs repos don't use the "<tag>: <description>." PR title format, only the "<description>" part. Descriptive tags are handled by the labels. (This is only worth mentioning because we generate our release notes automatically from the PR titles.)