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

DensityKind(::Type) #12

Closed
phipsgabler opened this issue Nov 28, 2021 · 3 comments
Closed

DensityKind(::Type) #12

phipsgabler opened this issue Nov 28, 2021 · 3 comments

Comments

@phipsgabler
Copy link
Contributor

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
Copy link
Collaborator

oschulz commented Nov 28, 2021

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
Copy link
Member

Can you use

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

or so?

I suggested something similar in TuringLang/DynamicPPL.jl#342 (comment) 🙂 (x should never be empty but maybe one could check it and throw an error if it is)

@phipsgabler
Copy link
Contributor Author

Okay then...

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

No branches or pull requests

3 participants