JuliaMath / DensityInterface.jl

Interface for mathematical/statistical densities in Julia
Other
12 stars 3 forks source link

`DensityKind(::Type)` #12

Closed phipsgabler closed 2 years ago

phipsgabler commented 2 years ago

I wonder why I have missed this before, but maybe it has been discussed somewhere... I currently want to implement dispatch like

check_tilde_rhs(x::AbstractArray{T}) where {T} = check_tilde_rhs(x, DensityKind(T))

which with the current interface doesn't work.

I'd have expected that DensityKind is defined on the type, and then defaults DensityKind(object) = DensityKind(typeof(object)), just like eltype. Yeah, this kind of defaulting is discouraged, but IMHO sufficiently close to eltype to make sense. I'd even be happy with only the type-based method, but extending the interface is backwards compatible. The only real change would be to require implementors define the type method, not the object method.

Is there any case where DensityKind(typeof(object)) doesn't make sense?

oschulz commented 2 years ago

Can you use

check_tilde_rhs(x::AbstractArray) = isempty(x) || check_tilde_rhs(x, DensityKind(first(x))

or so?

but maybe it has been discussed somewhere

Yes, we discussesd this, the conclusion was that the argument should be the value instead of the type (it had been the type in the first design iteration of DensityInterface).

Is there any case where DensityKind(typeof(object)) doesn't make sense?

I used a type-argument design for KernelAbstractions.get_device() initially, but Valentin Churavy asked me to change it and use the array instance instead of the type. He pointed out "One of the learnings for me from Adapt.jl is that you don't want to work on types, working on instances is more reliable." He should know ...

devmotion commented 2 years ago

Can you use

check_tilde_rhs(x::AbstractArray) = isempty(x) || check_tilde_rhs(x, DensityKind(first(x))

or so?

I suggested something similar in https://github.com/TuringLang/DynamicPPL.jl/pull/342#discussion_r757942678 :slightly_smiling_face: (x should never be empty but maybe one could check it and throw an error if it is)

phipsgabler commented 2 years ago

Okay then...