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

vardim keyword should belong to impute! call #32

Closed
nickrobinson251 opened this issue Jul 18, 2019 · 2 comments · Fixed by #38
Closed

vardim keyword should belong to impute! call #32

nickrobinson251 opened this issue Jul 18, 2019 · 2 comments · Fixed by #38

Comments

@nickrobinson251
Copy link
Contributor

On master, I was recently burned by this:

# I know I only ever want to impute with the mean
method = Impute.Fill(; value=mean);

# do some initial data stuff here...
# notice after processing the data has observations in the columns

# let's impute the missings!
impute!(X, method; vardim=1)
julia> impute!(X, method; vardim=1)
ERROR: MethodError: no method matching impute!(::Array{Float64,2}, ::Fill{typeof(mean)}; vardim=1)
Closest candidates are:
  impute!(::AbstractArray{T,2} where T, ::Impute.Imputor) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors.jl:71 got unsupported keyword argument "vardim"

i.e. I want to specify my imputation method up front, but I don't know until call time how my data will be stored

But it turns out I need this

# know up front how data is stored
method = Impute.Fill(; value=mean, vardim=1);

# do inital data stuff and make the data the way round the `method` requires

# now impute
impute!(X, method)

Similar use case is: I want to allow users to specify how to impute the data in the middle of some long data processing pipeline, without requiring them to know the orientation the data happens to have at the time imputation is performed.

@rofinn
Copy link
Member

rofinn commented Jul 23, 2019

The issue with this comes from the fact that our Impute.<method> functions need to both create the type using keywords and then call impute!. If we want to pass keywords to both then we'd need some way of figuring out which keywords should go to the type constructor and which should be passed to the impute! call. I suppose 1 option could be to use a default method that assumes keyword arguments to a type constructor match the field names and then pass the rest to the impute! method, but that seems hacky.

@nicoleepp
Copy link

Could have a kwarg for the keywords (as a Dict or namedtuple) that should go to the type contructor and pass the rest to the impute! method

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.

3 participants