Simplify Imputor API #69
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Replaced DropObs and DropVars in favour of a more generic Impute.Filter which isn't an imputor and requires `dims` to be specified for non-vector input.
2. Default colwise behaviour for matrices is deprecated in favour of `:` as the default. Folks may want to apply a method across all dimensions depending on the dataset.
3. `_impute!` methods should explicitly operate on `AbstractArray{Union{T, Missing}}`
4. Added `dims` option to the svd and knn imputors
5. Reworked the `knn` imputor a little as the code was overly complicated... may have also found a bug.
6. Added deprecation tests where possible to reduce testing noise
7. Everything should work on n-dimensional arrays now :)
TODO:
- Check on potential KNN bug
- Extend and breakup imputor tests
- Add n-dimensional array tests
- Make `Chain` not an imputor and have it work with Assertions, Imputors and Filter
rofinn
commented
Oct 1, 2020
…ng' values with actual `missing`s.
…stitute for substituting summary statistics.
|
Alright, this was bigger than expected, but a lot of the LOC changes are tied to removing the context blocks and moving a couple files around (also adding tests and docs). I've left several issues as TODO items to avoid making this bigger. @oxinabox Would you mind double checking that I'm using DataDeps.jl as expected? |
- Cleaned up docstrings for Imputors and functional calls - Reorganized the docs API section - Added 2 walkthroughs for SVD explicitly and working with a real spatiotemporal dataset - Added a DataDep for our test and demo datasets rather than relying on RDatasets and the corresponding dependencies
…umentation builds again.
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
reviewed
Oct 9, 2020
oxinabox
approved these changes
Oct 9, 2020
rofinn
commented
Oct 9, 2020
This was referenced Oct 10, 2020
|
Alright, I believe I've either addressed, responded or opened an issue for each comment left. Since there don't seem to be any new comments I'm gonna merge this and get started on the opened issues tomorrow. |
This was referenced Oct 16, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempting to resolve the following issues: #66, #57, #51, #50, #44, #43, #31
assertfunctioncontextcallsdimskeyword similar to the stats functionshttps://github.com/JuliaLang/Statistics.jl/blob/master/src/Statistics.jl#L164 (using special cased
:rowsand:colsdimsto improve readability)Drop in-place callsMight leave this for now_impute!(AbstractArray{Union{T, Missing}}, imp)dropobsanddropvarswithImpute.filterand adimskeywordChainnot an imputor and have it work onAssertionsandImputorsAdd function for checking if a methods support some input data? This might just be helpful for testing?Gonna leave this for now.