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

Define Random.Sampler #1314

Merged
merged 3 commits into from
May 2, 2021
Merged

Define Random.Sampler #1314

merged 3 commits into from
May 2, 2021

Conversation

devmotion
Copy link
Member

This PR defines Random.Sampler for Sampleables.

Motivation:
Recently I updated ConsistencyResampling and switched to the Random API. The package now works with all distributions for which the Random interface is implemented. Unfortunately, I noticed that it is not possible to use Distributions without committing type piracy since Random.Sampler is not implemented (an example of this "fix" is shown in https://devmotion.github.io/ConsistencyResampling.jl/dev/supported/#Vectors-of-distributions).

Follow-up:
There are some questions that could lead to additional PRs:

  • Is sampler still needed or should one switch to Random.Sampler completely and deprecate sampler?
  • Should one commit to the Random interface more closely? E.g., Random uses rand!(rng, x, sampler) whereas Distributions supports rand!(rng, sampler, x). Since it is not relevant for my use case I did not touch it in this PR but, if desired, one could implement rand!(rng, x, sampler) as well and possibly deprecate rand!(rng, sampler, x).

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2021

Codecov Report

Merging #1314 (95e7274) into master (99b2b57) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
+ Coverage   81.48%   81.51%   +0.02%     
==========================================
  Files         115      115              
  Lines        6623     6626       +3     
==========================================
+ Hits         5397     5401       +4     
+ Misses       1226     1225       -1     
Impacted Files Coverage Δ
src/genericrand.jl 100.00% <100.00%> (ø)
src/univariate/discrete/poisson.jl 70.14% <0.00%> (+0.45%) ⬆️
src/matrix/matrixnormal.jl 94.11% <0.00%> (+1.47%) ⬆️

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 99b2b57...95e7274. Read the comment docs.

@devmotion devmotion requested a review from matbesancon May 1, 2021 21:57
@matbesancon
Copy link
Member

All good here, I haven't looked at the API, but simplifying things in Distributions itself wouldn't hurt so if the Random.Sampler API is redundant we could only keep this one.
If we go in this direction, it needs to be done before 1.0. Could you add an issue to track this?

@devmotion
Copy link
Member Author

I opened an issue: #1316

@matbesancon
Copy link
Member

@devmotion we can merge this one too for the release

@devmotion
Copy link
Member Author

OK, I fixed the merge conflict. I can merge and tag 0.25 if the tests pass?

@matbesancon
Copy link
Member

yes absolutely

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.

3 participants