-
Notifications
You must be signed in to change notification settings - Fork 421
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
Consistent eltype
and allow to specify type in rand
#1433
Conversation
Thanks for PR and the careful writeup. I support these changes. |
I can see how this approach to I can understand |
I think these questions touch more general limitations of Random and of the Since Distributions (apart from LKJ) only implements Distributions of
|
Thanks @devmotion . As you probably guess, my main concern here is that we have something very similar in MeasureTheory, and many measures correspond to types other than arrays. Differences between Distributions and MeasureTheory aren't such a big deal, but it's convenient for users when there can be commonality. It had seemed at first you were suggesting an invariant like eltype(rand(::Type{T}, ::Sampleable)) == T But from this
it sounds like this could vary case by case, and may not be expected to satisfy any particular invariant. Is that right? |
I expect that every |
Coming from #1765 I am new to this long-standing discussion and my thinking is focused on continuous uniform distributions. Because the type of parameters and the type of the random numbers are two different things, we should keep them separate concepts. This, however, is not straightforward e.g. with AffineDistribution that transforms both parameter-derived quantities (like mode, ...) and samples. The specification of type of samples with the rand functions as proposed by @devmotion has its limitations where the user is not the direct caller of the rand function. E.g. with Turing, I specify How about introducing a second parametric type for the element-type Then we could have
and letting S default to Float64 or Int64 when not explicitly specified with the construction of the Distribution object, or for distributions where this parametric type is not yet implemented. The AffineDistribution can also receive two parametric types. Given its construction with parameters mu and sigma of type P and source distribution rho then I suggest:
If S is defined as element-type instead of the type of the entire sample, does it still confer the problems of sampling structured samples raised by @cscherrer? |
I agree with the suggestions from @bgctw. |
Well, on second thought, the counterargument is that it might be better to go with this interface, and then have Turing.jl allow users to specify sampling types that it then sends to Moving eltype into constructors themselves is more likely to propagate in weird ways to existing code, while the solution in this pull req is less likely to affect existing 3rd party code until explicit support is added (both a good thing and a bad thing, but probably more of a good thing overall since updating the big |
I don't think this is the right approach. At least for continuous distributions, IMO one should not encode the type of the samples in the distribution type - basically, similar to how typically in Julia functions are not typed with their return type. Trying to reason about return/sample types is brittle and prone to unexpected bugs, and it limits the dynamic nature of Julia. Similar to regular functions, IMO one should just push forward the result of the "basic" non-Distributions That being said, for discrete distributions it is a slightly different story - since the return type of |
Scope of the PR
The PR tries to address some of the problems with
rand
andeltype
- but it does not intend to solve all inconsistencies and problems. I try to emphasize this very strongly here because I really want to avoid that this PR ends up just like all the othereltype
discussions - which eventually means it will lead nowhere and nothing will change or be improved. It would be great if we could keep the discussion in this PR focused on the changes in this PR and on the question whether these changes are useful or not.In particular, this PR is not concerned with whether
eltype
should be replaced by e.g.Random.gentype
and/or return the type of the samples fromrand
instead of the element type of the samples (as it is documented and implemented currently). Hence I suggest discussing these things in other issues (probably for most things already an issue exists).Proposal
This PR is a proposal but not complete yet and would benefit from the generalizations of
rand
in #1391. However, I would like to start a discussion of the suggested changes.The PR
rand
, defined byeltype(::Type{<:Distribution})
, more consistent and defines it asInt
orFloat64
for most distributionsrand(::Type{T}, ::Sampleable, ...)
methods that allow to specify non-standard element types of samples (similar to e.g.rand(T)
)The first change is already the default currently for many distributions and affects only the odd implementations of
Normal
,MvNormal
,MvNormalCanon
,MvLogNormal
,GenericMvTDist
,Dirichlet
, andLKJCholesky
. Hence it could be viewed as bugfix but I think it is safer to make a breaking release. The second change is non-breaking.Fixes #1071, #1082, #960, #1041, #1355, #1045.
Fixes #1163 and #821:
Motivation
There are many open and closed issues and PRs about the current
eltype
situation. One issue is that some distributions such asNormal
defineeltype
based on the parameters whereas most distributions defineeltype
without taking them into account. In the first case hence one can change the type of the parameters of a distribution to achieve thatrand
returns samples of a different type whereas in the latter case one can't. Thus the definition ofeltype
is directly related to the question of how one can obtain samples whose element types are notFloat64
orInt
, the default types forContinuousDistribution
andDiscreteDistribution
.I argue that often it is not reasonable to define
eltype
based on the parameters of a distribution and instead it is necessary to be able to specify the type of the samples separately. For instance, usually the parameter of aPoisson
distribution is a floating point number but the samples are integer-valued and hence often expected to be of typeInt
. It does not seem possible to set and adjust the type of the samples based on the parameter type.Another indication that the current design is a bit ad-hoc is that e.g. in the odd case of
Normal
which defineseltype
as the type of the parameters (i.e., identical topartype
which is the interface function for the type of the parameters)eltype
may returnInt
: One can define aNormal
distribution with integer-valued parameters which is useful e.g. if one wants to perform calculations, e.g., oflogpdf
, with pointsx
of different types such asFloat32
andFloat64
and wants to avoid undesired type promotions. However, clearly the samples from aNormal
distribution with integer-valued parameters are not integers. Currently, we use the workaroundfloat(eltype(d))
and return samples of typeFloat64
in this case. But this is just a heuristic and so while we avoid promotions in e.g.logpdf
it does not allow to generate samples of typeFloat32
if the parameters are integer-valued.Additionally, the current design is inconsistent as noted in many issues and PRs: Why should the parameters of
Normal
affect the type of samples fromrand
but e.g. samples fromUniform
are always of typeFloat64
, independent of the parameters?These observations motivate the proposal in this PR to
eltype
for most distribution in a parameter-independent way (default:Float64
andInt
forContinuousDistribution
s andDiscreteDistribution
s)rand
call directly.The first point fixes the inconsistent behaviour of
Normal
,MvNormal
,MvLogNormal
, andDirichlet
. It allows us to remove thefloat(eltype(...))
heuristic completely. The second point ensures that it is still possible to adjust the types of the samples. However, now this can be done independent of the parameters, e.g. one does not have to change the parameters toFloat32
to obtainFloat32
samples and one can generateFloat32
samples from integer-valuedNormal
distributions orPoisson
distributions with a parameter of typeFloat64
.There are some distributions though for which it makes sense and is even required to base the default type of the samples on some parameter:
Dirac
: it seems reasonable to "sample" the value of theDirac
distribution by returning its parameter without any conversions (the parameter can be anyReal
and is also not restricted toInt
)DiscreteNonParametric
: similarly, it makes sense to sample from the providedsupport
without conversion (again the values are also not necessarily of typeInt
)LocationScale
forDiscreteDistribution
s: Scaling and shifting can promote the type of the support of aDiscreteDistribution
e.g. toRational
s, and hence the default type of the samples should take into account the parameters ofLocationScale
and not be set to the sample type of the unscaled distribution (which could be e.g. of typeInt
)Alternatives
The second point (
rand(::Type{T}, ::Sampleable)
) seems generally useful, less controversial, and non-breaking. Hence I did not consider any alternative there.The first point is more controversial as it changes e.g. the type of samples from
Normal(0f0)
and hence, in my opinion, is breaking (if one does not consider it as a bugfix). An alternative would be e.g. to defineeltype
based on the parameters if the parameters are clearly and directly related to the support: E.g.,Normal
eltype
would still be based on the parameters as the mean parameter is directly related to the support,eltype
ofUniform
would be changed such that it is based on the type of the bounds which define the support,eltype
ofBeta
would be set toFloat64
and ofPoisson
toInt
since its parameters are not directly related to the supportHowever, this would not fix the
float(eltype(...))
and one would still have to deal with non-floating point parameters. An approach could be to include it in the definition ofeltype
directly instead of applying it as a heuristic in therand
calls: E.g., one could defineBase.eltype(::Type{Normal{T}}) where {T} = float(T)
- the parameter type could still be retrieved unmodified withpartype
.I am not sure if this would be more confusing since the definition of
eltype
would be less consistent. Also maybe sometimes it could be a bit difficult to decide ifeltype
should be based on the parameters or not. On the other hand, maybe it would be more intuitive since e.g. there are already two open PRs that want to defineBase.eltype(::Type{Uniform{T}}) where {T} = float(T)
(in the PRs probablyT
but again parameters don't have to be floating point numbers). Possibly therefore such a change would be less surprising and could be viewed as a non-breaking bugfix.