search

invenia / Impute.jl

Imputation methods for missing data in julia
https://invenia.github.io/Impute.jl/latest/
Other
77 stars 11 forks source link

Fix broken tests #58

Closed appleparan closed 4 years ago

appleparan commented 4 years ago

Some tests are broken due to use of outdated API. Let's fix it.

rofinn commented 4 years ago

Can you clarify what you mean? If I recall the only broken tests involve attempts to mutate matrices and tables in cases where that won't work. I'm tempted to drop the current mutating API in favour of supporting separate iterative and static methods. If folks want to mutate the original data they can choose write the results back to the original data if they wish.

appleparan commented 4 years ago

There are some warning messages in CI. [Link] The warnings are not only complaining not mutating data, but also deprecation. That's the point i want to fix.

By the way, I also want to ask this. Why do you want to drop mutating API? If you don't want to support mutating data, Imputation may require new allocation(!) and this could be huge loss of performance and memory. However, for iterative methods, I agree with your idea. This is not unavoidable. I think static methods may have current mutating API, but iterative method's impute! should write result back to the original data.

rofinn commented 4 years ago

Ahh, okay. Yeah, my plan was to finish removing those deprecations after the next release. I don't want to remove the tests until we're ready to remove support completely. You can always start julia with --depwarn=no.

The problem is that mutability doesn't generalize well for all possible input types. We've had issues with mutability assumptions for specific table and array types (e.g., https://github.com/JuliaData/Tables.jl/issues/116) and we have tests that demonstrate this failure condition.