Simplify tests and do not use eval
#88
Conversation
|
Zygote tests passed (after 30 minutes instead of 1h 48min on master 😃), which indicates that #87 is fine (Zygote tests there are still running). |
|
So this looks all good to me, but that's because I already saw the same thing earlier 😅 Should I just accept it or should maybe someone else also have a look, just to be sure? |
|
I requested a review from @mohamed82008 as well since I assume he implemented the current test setup. |
|
thanks, @devmotion -- nice improvements! |
|
Sorry guys I was moving houses and had internet issues. Had a quick scan and looks good though. |
| end | ||
|
|
||
| if isempty(θ) | ||
| # In this case we can only test the gradient with respect to `x` |
There was a problem hiding this comment.
We should test wrt to x only even if theta is not empty.
There was a problem hiding this comment.
And we should check if the support is continuous first.
There was a problem hiding this comment.
The logic is just copied from the previous implementation. There we test wrt x if theta is empty and don't check if the support is continuous (see e.g. https://github.com/TuringLang/DistributionsAD.jl/blob/master/test/test_utils.jl#L126 - somehow I forgot to delete the file...). I guess it's OK to not check if the support is continuous, since otherwise we couldn't test any derivatives at all and the DistSpec would not be useful. Additionally, both in the old and new implementation we test wrt x if theta is not empty and the support is continuous.
There was a problem hiding this comment.
Even if the old implementation didn't test wrt x only with a non-empty theta, we still should. This is useful for prior distributions in Turing.
There was a problem hiding this comment.
Hmm I can't follow, why would we want to remove tests? As long as tests pass in a reasonable amount of time (which they do with the new implementation), it should always be better to perform more tests, shouldn't it?
There was a problem hiding this comment.
Yes I am suggesting performing more tests :) Currently I don't think we test wrt to x and only x when theta is not empty.
There was a problem hiding this comment.
Ah I see, somehow I didn't get it and assumed you would want to remove some of the existing tests 😆
This PR is basically a slightly modified copy of TuringLang/Bijectors.jl#116. Instead of FiniteDiff it uses FiniteDifferences (since DistributionsAD uses it already), and I added some additional transformations to ensure that parameters and samples are always in the correct domains.
This PR is based on #87 since it seemed weird to keep the special non-randomized tests for
Dirichletthat couldn't capture the bug in #86.