Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default rng on 1.7 up #220

Merged
merged 2 commits into from
Feb 20, 2022

Conversation

isaacsas
Copy link
Member

No description provided.

@isaacsas isaacsas changed the title Change default rng Change default rng on 1.7 up Feb 20, 2022
@ChrisRackauckas
Copy link
Member

Did you get a rough estimate of how much faster this is now?

Should DiffEqNoiseProcess.jl also change its defaults (for SDEs)?

@isaacsas
Copy link
Member Author

I haven't benchmarked yet, so I can't say. But I think we should switch anyways since the default rng should be better from a statistical point of view too (as I understand it).

@isaacsas
Copy link
Member Author

I always had concerns about the *128 family as some members failed some of the standard test suites (I never found anything though about the one we were using, hence why I left it).

@isaacsas
Copy link
Member Author

A very simple test:

using Random, RandomNumbers, BenchmarkTools

N = 1000000

function bdpath(N,rng)
    x = 0.0
    for i = 1:N
        x += rand(rng)
    end
end

rng1 = Random.default_rng()
rng2 = Xorshifts.Xoroshiro128Star(rand(UInt64))

b1 = @benchmark bdpath($N,$rng1)
b2 = @benchmark bdpath($N,$rng2)

display("Julia:")
@show median(b1)

display("Xorshift:")
@show median(b2)

gives

"Julia:"
median(b1) = TrialEstimate(997.375 μs)

"Xorshift:"
median(b2) = TrialEstimate(1.571 ms)

on my M1 MBA with 1.7.1.

Using randn instead of rand gives

"Julia:"
median(b1) = TrialEstimate(3.684 ms)
"Xorshift:"
median(b2) = TrialEstimate(3.841 ms)

so less of an improvement there.

@ChrisRackauckas
Copy link
Member

Still pretty decent. Add in the thread safety and 👍 this is great.

@isaacsas
Copy link
Member Author

Yeah, that was great improvement in base.

@ChrisRackauckas ChrisRackauckas merged commit 3a59d4e into SciML:master Feb 20, 2022
@isaacsas isaacsas deleted the change_default_rng branch February 20, 2022 23:53
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.

None yet

2 participants