clean up mean_function tests without reactivating AD #315
Conversation
Codecov Report
@@ Coverage Diff @@
## master #315 +/- ##
=======================================
Coverage 97.64% 97.64%
=======================================
Files 10 10
Lines 382 382
=======================================
Hits 373 373
Misses 9 9 Continue to review full report at Codecov.
|
| @@ -1,47 +1,53 @@ | |||
| @testset "mean_functions" begin | |||
There was a problem hiding this comment.
A bit simpler to just generate them once:
| @testset "mean_functions" begin | |
| @testset "mean_functions" begin | |
| Random.seed!(123456) | |
| N = 5 | |
| D = 3 | |
| x1 = randn(N) | |
| xD = ColVecs(randn(D, N)) | |
| xD′ = RowVecs(randn(N, D)) |
The Mersenne RNG is not the default anymore, so it seems slightly strange to enforce it explicitly. In general, RNGs are reset after each testset so it should be sufficient to just set the seed once at the top/in runtests.jl but that's not part of the PR.
There was a problem hiding this comment.
If the exact random numbers matter, then it makes sense to explicitly state which random number generator you use (otherwise your tests might suddenly fail once julia moves to a new default).
If the exact random numbers don't matter, then it shouldn't even need the Random.seed! ... no?
There was a problem hiding this comment.
No. The random numbers may be (and probably are) different for different Julia versions and hardware architectures anyway. Setting a seed is only useful to avoid spurious test failures with numerically bad samples - they are quite rare here I assume but without any seed we might eventually get them in some test run.
There was a problem hiding this comment.
So, hypothetically, you might still eventually get that issue at some julia version upgrade.
What's the problem with keeping the MersenneTwister then ? Why would it be needed that I add this one additional change?
There was a problem hiding this comment.
It's not strictly necessary but a) it seems simpler and b) allows us to test the methods with the default, and hence most likely used, RNGs. b) can also lead to a speed-up in more recent Julia versions.
There was a problem hiding this comment.
I don't think it should be necessary in this PR. please change separately if you care.
There was a problem hiding this comment.
I really don't understand why you're so hesitant to take up any suggestions and basically always suggest to "just do it later". This is very frustrating, and in my opinion also unproductive. In this PR these lines are changed so it's the perfect opportunity for such improvements, in particular if they are so simple.
There was a problem hiding this comment.
Just clean something up as I go along is something that I thought at other times, too, only to get a comment from you complaining that I changed something else.
I'm hesitant to take up your suggestions because it's more work. Sure I could accept your suggestion, then I have to go and fix the other uses of rng. Maybe it would make it even better than what I proposed. But I don't have the time to make everything perfect! It gets to the point where it seems that proposing any change ends up taking hours of back and forth, even though it would have already improved a bit over the status-quo already.
The measure for whether a PR should be merged or not shouldn't be "is this the best it could possibly be?", only "does this improve over what we had before?" (implying it doesn't make anything else worse in the process).
| rng = MersenneTwister(123456) | ||
| N, D = 5, 3 | ||
| x1 = randn(rng, N) | ||
| xD_colvecs = ColVecs(randn(rng, D, N)) | ||
| xD_rowvecs = RowVecs(randn(rng, N, D)) |
There was a problem hiding this comment.
| rng = MersenneTwister(123456) | |
| N, D = 5, 3 | |
| x1 = randn(rng, N) | |
| xD_colvecs = ColVecs(randn(rng, D, N)) | |
| xD_rowvecs = RowVecs(randn(rng, N, D)) | |
| Random.seed!(123456) | |
| N, D = 5, 3 | |
| x1 = randn(N) | |
| xD_colvecs = ColVecs(randn(D, N)) | |
| xD_rowvecs = RowVecs(randn(N, D)) |
|
I unsubscribed, please find another reviewer. |
|
Many thanks, @st-- for the nice improvements! I think it's ready to get merged. Also many thanks to @devmotion for the thorough and thoughtful reviews all the time. |
No description provided.