Remove use of eval in tests
#116
Conversation
Codecov Report
@@ Coverage Diff @@
## master #116 +/- ##
=======================================
Coverage 57.22% 57.22%
=======================================
Files 23 23
Lines 1169 1169
=======================================
Hits 669 669
Misses 500 500 Continue to review full report at Codecov.
|
|
With this PR Zygote tests finish in around 25 minutes instead of > 3h on master. It seems that Julia 1.0 requires some additional adjoints for Tracker and ReverseDiff though. |
|
Travis tests fail probably due some change introduced in Tracker 0.2.7 (the previous commit passed on Tracker 0.2.6, and the last commit did not change anything that affects the Travis tests). |
|
@torfjelde Any idea what change in Tracker (or some other dependency) might cause the issues on Travis? Otherwise the tests pass and are extremely fast now 🎉 I guess, the tests in DistributionsAD should be changed in the same/similar way. |
|
BTW if someone enables codecov for this repo, then the codecov comments in the PRs provide some more meaningful information about coverage changes. |
Isn't Codecov already enabled? |
|
Hmm maybe it is actually. I was confused that the report states that the coverage diff is "n/a" but maybe that's not relevant. |
It's indeed strange. Codecov page says there aren't sufficient commits for this repo yet. @torfjelde Please add a code review, or proceed with merging if everything looks good. |
|
I assume the issues on Travis are the same as in FluxML/Tracker.jl#71. It seems the latest Tracker release is declared to be compatible with Adapt 2 but was only tested with Adapt 1. |
|
Tests on master fail due to the same problem. |
There was a problem hiding this comment.
This is great @devmotion ! Looks much better; thank you!:)
Have a couple of questions, but overall looks good 👍
EDIT: Don't merge until my comments have been addressed! Shouldn't have "approved" before we've removed the redundant comments and discussed the matrix-case a bit.
| # Matrix case doesn't work for continuous distributions for some reason | ||
| # now but not too important (?!) | ||
| if length(sz) == 2 && Distributions.value_support(typeof(d)) === Continuous | ||
| continue | ||
| end |
There was a problem hiding this comment.
Do you know why? I.e. what's the error?
There was a problem hiding this comment.
No, I can check it. That's why I added "(?!)" to the comment 😄
There was a problem hiding this comment.
BTW I think it's pretty bad to just state arrays of distributions for which tests are broken because we won't notice if some change (in Bijectors or some other package) fixes a broken test. IMO we should explicitly test whatever behaviour is broken. I just didn't want to put too many changes in this PR and wanted to check if it actually helps to reduce testing times first.
There was a problem hiding this comment.
It seems it's mainly an issue with Tracker (yields
MethodError: Cannot `convert` an object of type Array{Float64,1} to an object of type SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}already for the first distribution), although Zygote also yields errors for some distributions such as arraydist with GeneralizedExtremeValue (but works fine for all distributions above, i.e., Arcsine to Gamma).
There was a problem hiding this comment.
Sounds like a good idea, but how would we do this in practice when checking PRs and stuff? Should have a one test-suite which tests broken tests and is allowed to fail? Doesn't seem like that will helps us discover if tests are fixed by upstream packages since there will be too much noise.
There was a problem hiding this comment.
I guess we would have to state explicitly for which AD backends tests don't pass, and then use @test_broken instead of @test for these inside of test_ad. Probably would need an additional argument to DistSpec, e.g., a field broken with default value () that could be set to ("Zygote",), ("Zygote", "ReverseDiff") etc. Would be tedious to set up initially, but should work quite reliably I assume.
There was a problem hiding this comment.
Aaaah, pfft I haven't read the docs of @test_broken before; that makes complete sense. But should we maybe just get this PR merged now and then we can add this later? Unless you're really keen on just getting it done immediately, in which case I won't stop you:)
There was a problem hiding this comment.
No, actually I'm not very keen on going through the lists and figuring out why these tests are broken right now 😛 So I'm fine with adding this later.
|
Merged; thanks @devmotion !:) |
This PR removes the use of
evalin the tests, simplifies their structure, and avoids some symmetries and "nice" specially chosen values in the AD tests by using random parameters and samples.