Skip to content

Conversation

@mtsch
Copy link
Contributor

@mtsch mtsch commented Aug 1, 2021

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
Copy link

codecov-commenter commented Aug 1, 2021

Codecov Report

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

❗ Current head ee13398 differs from pull request most recent head 2e599d9. Consider uploading reports for the commit 2e599d9 to get more accurate results
Impacted file tree graph

@@            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%) ⬇️
src/Xorshifts/xoshiro256.jl 97.36% <85.71%> (+0.14%) ⬆️
src/Xorshifts/splitmix64.jl 100.00% <100.00%> (ø)
src/Xorshifts/xoroshiro128.jl 96.29% <100.00%> (+7.01%) ⬆️
src/Xorshifts/xoroshiro64.jl 96.42% <100.00%> (+0.13%) ⬆️
src/Xorshifts/xorshift1024.jl 97.77% <100.00%> (+2.03%) ⬆️
src/Xorshifts/xorshift128.jl 96.15% <100.00%> (+0.15%) ⬆️
src/Xorshifts/xorshift64.jl 100.00% <100.00%> (ø)
src/Xorshifts/xoshiro128.jl 97.36% <100.00%> (+0.14%) ⬆️
... 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 sunoru merged commit 18eccc4 into JuliaRandom:master Aug 1, 2021
@sunoru
Copy link
Member

sunoru commented Aug 1, 2021

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

@ChrisRackauckas
Copy link
Contributor

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
Copy link
Member

sunoru commented Aug 1, 2021

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 #78 can be prevented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants