mtsch commented 3 years ago

I'm sorry about the last PR I made, it contained the wrong fix. This PR fixes it.

Can I add some kind of test to make sure this doesn't happen again? I don't see exactly how it would fit with the current tests.

codecov-commenter commented 3 years ago

Codecov Report

Merging #83 (ee13398) into master (dcc114a) will decrease coverage by 2.01%. The diff coverage is 98.55%.

:exclamation: Current head ee13398 differs from pull request most recent head 2e599d9. Consider uploading reports for the commit 2e599d9 to get more accurate results

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   94.37%   92.35%   -2.02%     
==========================================
  Files          22       20       -2     
  Lines         569      589      +20     
==========================================
+ Hits          537      544       +7     
- Misses         32       45      +13

Impacted Files	Coverage Δ
src/Xorshifts/common.jl	`100.00% <ø> (ø)`
src/Xorshifts/docs.jl	`0.00% <ø> (-100.00%)`	:arrow_down:
src/Xorshifts/xoshiro256.jl	`97.36% <85.71%> (+0.14%)`	:arrow_up:
src/Xorshifts/splitmix64.jl	`100.00% <100.00%> (ø)`
src/Xorshifts/xoroshiro128.jl	`96.29% <100.00%> (+7.01%)`	:arrow_up:
src/Xorshifts/xoroshiro64.jl	`96.42% <100.00%> (+0.13%)`	:arrow_up:
src/Xorshifts/xorshift1024.jl	`97.77% <100.00%> (+2.03%)`	:arrow_up:
src/Xorshifts/xorshift128.jl	`96.15% <100.00%> (+0.15%)`	:arrow_up:
src/Xorshifts/xorshift64.jl	`100.00% <100.00%> (ø)`
src/Xorshifts/xoshiro128.jl	`97.36% <100.00%> (+0.14%)`	:arrow_up:
... and 11 more

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 7bf44b5...2e599d9. Read the comment docs.

sunoru commented 3 years ago

Ah, I see. I shall later add this into tests.

ChrisRackauckas commented 3 years ago

I'm not sure these seeds are correct. I'm seeing subtle differences causing test errors:

https://github.com/SciML/DiffEqSensitivity.jl/pull/459/checks?check_run_id=3214665808#step:6:264

I'd assume the seed changes would not have changed the actual random numbers from v1.4?

sunoru commented 3 years ago

Sorry I didn't mention in the release note. v1.5 uses different seeds from v1.4 for simple integer input, by applying splitmix64 before seeding. Thus, issue like https://github.com/JuliaRandom/RandomNumbers.jl/issues/78 can be prevented.

JuliaRandom / RandomNumbers.jl

Fix `seed!` for Xoshiro256 again #83

Codecov Report