-
Notifications
You must be signed in to change notification settings - Fork 415
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
Decouple rand
and eltype
#1905
base: master
Are you sure you want to change the base?
Decouple rand
and eltype
#1905
Conversation
747a191
to
0ea5502
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1905 +/- ##
==========================================
- Coverage 86.08% 85.60% -0.49%
==========================================
Files 144 144
Lines 8696 8758 +62
==========================================
+ Hits 7486 7497 +11
- Misses 1210 1261 +51 ☔ View full report in Codecov by Sentry. |
Given that, according to the docstring Distributions.jl/src/common.jl Lines 96 to 102 in a1010e4
the sole purpose of eltype is to return the type of rand , which I agree isn't really feasible, should we also deprecate the methods here as part of this PR?
|
#1907 is related. |
5644af6
to
3e66d3e
Compare
My personal opinion: This is orthogonal to the fact that Distributions should probably support In an ideal situation, I think we would define |
I became convinced that in general this is not possible. It basically means trying to manually figure out type inference, a task that even the Julia compiler can fail at. Of course, for some number types and distributions it can be done (as also the compiler will succeed in many instances). But in general it's far from trivial. As in the initial stages of Distributions, one could be restrictive and limit samples to |
4db3858
to
d824cf8
Compare
d824cf8
to
2e7752e
Compare
One of my take-aways from issues such as #1252, #1902, #894, #1783, #1041, and #1071 is that
eltype
is not only quite inconsistently implemented and unclearly documented currently but more generally it might be a bad design decision to use it for pre-allocating containers of samples: Setting it to a fixed type (historicallyFloat64
for continuous andInt
for discrete distributions) is too limiting, but trying to infer it from parameters is challenging and doomed to fail for more challenging scenarios that are not as clear as e.g.Normal
.This PR tries to decouple
rand
andeltype
, to make it easier to possibly eventually deprecate and removeeltype
.Fixes #1041.
Fixes #1071.
Fixes #1082.
Fixes #1252.
Fixes #1783.
Fixes #1884.
Fixes #1902.
Fixes #1907.