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

Check is_transformable via duck typing #74

Closed
glennmoy opened this issue Apr 7, 2021 · 2 comments · Fixed by #83
Closed

Check is_transformable via duck typing #74

glennmoy opened this issue Apr 7, 2021 · 2 comments · Fixed by #83

Comments

@glennmoy
Copy link
Member

glennmoy commented Apr 7, 2021

The transform API is currently checked by calling is_transformable on a type, which just checks for types that are expected to have an apply method defined for it. This is just duck typing but with extra steps.
This way of checking the interface is brittle and unnecessary: if users define an apply method for their type downstream but forget to extend is_transformable for it it defeats the purpose, similarly one can easily get around the check by extending is_transformable without actually extending apply.
Rather, is_transformable(my_type) should just verify that an apply method for my_type exists and cut out this middle step.

@glennmoy glennmoy changed the title Better way to check is_transformable? Check is_transformable via duck typing Apr 7, 2021
@bencottier
Copy link
Contributor

I thought

is_transformable(::T) where T = hasmethod(apply, Tuple{T, Transform})

But that allows anything for T because of the table method. How do we deal with that?

@glennmoy
Copy link
Member Author

glennmoy commented Apr 16, 2021

I thought

is_transformable(::T) where T = hasmethod(apply, Tuple{T, Transform})

But that allows anything for T because of the table method. How do we deal with that?

We can probably work around it, for example methodswith can restrict to not checking against the supertypes including Any.

help?> methodswith
search: methodswith

  methodswith(typ[, module or function]; supertypes::Bool=false])

  Return an array of methods with an argument of type typ.

  The optional second argument restricts the search to a particular module or function (the default is all top-level
  modules).

  If keyword supertypes is true, also return arguments with a parent type of typ, excluding type Any.

So in this case something like this would work:

function is_transformable(T::Type) 
    Tables.istable(T) && return true  # cannot check against the method with no type annotation
    return !isempty(methodswith(T, FeatureTransforms.apply; supertypes=true))
end
julia> trans = [[1, 2, 3], [[1 2], [3 4]], (a=[1, 2], b=[3, 4]), DataFrame(:a=>[1, 2], :b=>[3, 4])];

julia> all(is_transformable(t) for t in trans)
true

julia> all(is_transformable(typeof(t)) for t in trans)
true

# extend it for a new type
julia> FeatureTransforms.apply(x::Integer, ::Transform) = x

julia> transformable(1)
true

But I'm not sure how effective it is to rely on the method table for these things.

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 a pull request may close this issue.

2 participants