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

faster randmtzig_exprnd (separate unlikely branch + inline) and rename it "randexp" #9144

Merged
merged 1 commit into from
Nov 30, 2014

Conversation

rfourquet
Copy link
Member

Same transformation as was done recently on randn.
The rename was suggested by @ViralBShah. It turns out that this function is not exported by Base (nor documented it seems), should it be? If yes, I will write the array version randexp! too.

@simonbyrne
Copy link
Contributor

+1 for the rename.

@simonbyrne
Copy link
Contributor

Also, I realise it's unexported, but it is used by Distributions, so a @deprecate would be useful too.

@ivarne
Copy link
Member

ivarne commented Nov 25, 2014

Just don't use the @deprecate macro, because it automatically exports the now deprecated function.

@lindahua
Copy link
Contributor

If you make this renaming, I'd suggest to backport it to 0.3.

As @simonbyrne, Distributions.jl relies on this function.

@ViralBShah
Copy link
Member

We can't change the 0.3 api. Best thing would be to deprecate, or handle it in distributions.jl with a version check. Thoughts?

@simonbyrne
Copy link
Contributor

There's also Compat.jl.

@ViralBShah
Copy link
Member

Compat.jl works. @simonbyrne @lindahua What would you guys prefer? @ivarne Would using compat.jl overcome the export issue?

@ViralBShah
Copy link
Member

@rfourquet Could you also add the array fill versions for randexp also like for randn? Also, any good ideas on testing randn and randexp, both the scalar and the vector versions?

@ViralBShah ViralBShah added the randomness Random number generation and the Random stdlib label Nov 26, 2014
@lindahua
Copy link
Contributor

Compat.jl is fine. Or you may also let me know the exact version number where this changes.

@ViralBShah
Copy link
Member

Ok - Compat.jl it is then.

@rfourquet
Copy link
Member Author

I have no "good" ideas for tests, but can at least add a test that the API works. I'm adding the array version, should we export those functions then?

@ViralBShah
Copy link
Member

We just have to export randexp and randexp!, right?

@rfourquet
Copy link
Member Author

OK so I exported them, please check my wording in the documentation. I don't know about Compat.jl, must I do something?

@tkelman
Copy link
Contributor

tkelman commented Nov 26, 2014

Compat.jl is an external package at https://github.com/JuliaLang/Compat.jl, so breaking changes can be made on base and Compat.jl will have to add some code to allow packages to work for both 0.3 and 0.4 by using the same syntax from Compat.jl.

So if you're feeling particularly generous you can prepare a PR to Compat.jl that manages to work the same way both before and after this change.

@ViralBShah
Copy link
Member

Basically, Distributions.jl uses the internal randmtzig_exprnd function. Now, Distributions.jl will be updated to use randexp, which will break with 0.3. Compat.jl will basically make sure that on 0.3, we have a randexp function that calls the old one, and on 0.4, it continues to call randexp without any modifications. Examples in the Compat package should give you a good idea.

@rfourquet
Copy link
Member Author

Thanks, so I understand that I just need to wait this to be merged to know the VERSION before issuing a PR to Compat.jl (VERSION is not yet clear to me: today's master is v"0.4.0-dev+314", whereas 2 days old master shows v"0.4.0-dev+1763", which is greater).

@ivarne
Copy link
Member

ivarne commented Nov 26, 2014

The last number in the version is the number of commits since last tagged version on the current branch. If you have the versions you say, that is a bug. Can you check it again?

@rfourquet
Copy link
Member Author

Thanks @ivarne, it's just that I had used a light tag for personal use, this is fixed if I remove it.

faster randexp (separate unlikely branch + @inline)
add array versions of randexp and export these functions
@ViralBShah
Copy link
Member

I squashed these and am merging. @rfourquet Could you issue the PR against Compat.jl? You should have commit access there, so you can just create a branch in Compat.jl.

ViralBShah added a commit that referenced this pull request Nov 30, 2014
faster randmtzig_exprnd (separate unlikely branch + inline) and rename it "randexp"
@ViralBShah ViralBShah merged commit cb75579 into master Nov 30, 2014
@rfourquet
Copy link
Member Author

See JuliaLang/Compat.jl#21.

@rfourquet rfourquet deleted the rf/randexp branch December 1, 2014 05:01
@ViralBShah
Copy link
Member

@simonbyrne @lindahua Once this PR is merged, would you be able to update Distributions.jl?

randexp(MersenneTwister(10))
randexp(MersenneTwister(10), 100000)
randexp!(MersenneTwister(10), Array(Float64, 100000))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably @test something too. This code will not verify anything other than that it doesn't throw exceptions. I know the results are random, but the size, type and range is not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I only duplicated what already existed for randn, but I agree that testing values should be added.
I personally find that it is good to test somewhere only the API (smaller sizes than 100000 would do it though). What we could do here is test that the global version and the version with a passed rng give the same results. I am planning to rewrite #8339 differently, with all API checks grouped together. Then it could be enough to do all the other tests on the global RNG.

StefanKarpinski pushed a commit that referenced this pull request Feb 8, 2018
add compatibility for randexp (Julia PR #9144)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants