CompatHelper: bump compat for "Distributions" to "0.24" #119
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
devmotion
force-pushed
the
compathelper/new_version/2020-10-08-00-20-15-256-1076411718
branch
from
03ef0b7 to
0a7f688
Compare
|
Test errors are real and caused by the changes in Distributions 0.24. |
|
@mohamed82008 Any idea what could cause the test errors of Skellam with ReverseDiff? I'm particularly confused since the univariate and matrixvariate tests of Skellam pass but the multivariate filldist/arraydist tests fail. Could it be due to the changes in Distributions 0.24? Or some changes in ReverseDiff? |
|
Sorry I totally missed your comment here. I will take a look. |
|
This PR is blocking other PRs (#122 and JuliaGaussianProcesses/AbstractGPs.jl#73) and contains fixes that block #120 (could be extracted to a separate PR though). To resolve this situation, I marked the failing multivariate tests of Skellam with ReverseDiff as broken. I suggest to investigate and fix this issue separately later on since it seems it is still not clear what causes these new test failures. |
|
Haha 🤣 In the mean time, the Skellam issue seems to be fixed due to some change upstream and JuliaStats/Distributions.jl#1198 breaks PoissonBinomial for Zygote. |
This reverts commit c63b814.
|
Ah wait, it seems that only some of the Skellam tests in the multivariate case are broken... |
|
Any progress on this @devmotion @mohamed82008? Keen to get this resolved for the sake of AbstractGPs |
|
I'm just waiting for a review by @mohamed82008 (or someone else). |
|
On it. |
| ) | ||
| return sum(x -> logpdf(d, x), X) | ||
| # Only needed in Distributions < 0.24 | ||
| if !DISTRIBUTIONS_HAS_GENERIC_UNIVARIATE_PDF |
There was a problem hiding this comment.
Why is this not needed? Please provide an explanation and a benchmark to make sure there is no regression especially for ReverseDiff and Zygote.
There was a problem hiding this comment.
Because Distributions implements this now for any MatrixDistribution (as it should): https://github.com/JuliaStats/Distributions.jl/blob/ae2d6c5c68abc0aeafdc249ca60df058c19279cb/src/matrixvariates.jl#L225-L232
I don't think a benchmark is useful here since it is exactly the same implementation.
There was a problem hiding this comment.
Can we just make this a check of the Distributions version?
There was a problem hiding this comment.
AFAIK there's no official API or way to check the version of certain packages, that's why DISTRIBUTIONS_HAS_GENERIC_UNIVARIATE_PDF serves as a proxy (another example is https://github.com/TuringLang/Turing.jl/blob/beb440593b7a61f299a875abb84f3521fdbf3ada/src/Turing.jl#L61-L66).
|
Looks good to me in general. Just please address my minor comments above. |
|
@mohamed82008 I don't think it is possible to address #119 (comment). The other comments I addressed and marked as resolved. |
|
Great work @devmotion! Thanks for the PR. |
delete-merged-branch
bot
deleted the
compathelper/new_version/2020-10-08-00-20-15-256-1076411718
branch
This pull request changes the compat entry for the
Distributionspackage from0.23.3to0.23.3, 0.24.This keeps the compat entries for earlier versions.
Note: I have not tested your package with this new compat entry. It is your responsibility to make sure that your package tests pass before you merge this pull request.