Refactor transformed estimators #52
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
This mixin provides two methods (`predict_independent` and `score_independent`) that don't include the query point itself when finding nearest neighbors. These methods are similar in functionality to calling `KNeighborsRegressor.kneighbors()` without passing `X`. In this way, only "independent" neighbors are used in calculating predictions and scores. This PR is intended to fix #45.
* Replace IndependentPredictionMixin functions with a fit function that sets estimator attributes * Change tests to compare against estimator attributes for independent_prediction_ and independent_score_ * Clean up variable names used in tests * Simplify logic in test_predict for choosing the expected prediction data
Previously, the core estimators were using three mixins (`KNeighborsDFIndexCrosswalkMixin`, `TransformedKNeighborsMixin`, `IndependentPredictionMixin`) and the core `sklearn.neighbors.KNeighborsRegressor` as superclasses. This commit introduces a new subclass (`_KNeighborsRegressor`) that inherits from `KNeighborsDFIndexCrosswalkMixin` (renamed to `DFIndexCrosswalkMixin`), `IndependentPredictionMixin`, and `sklearn.neighbors.KNeighborsRegressor. Additionally, the functionality in `TransformedKNeighborsMixin` is now presented in a subclass of `_KNeighborsRegressor` renamed to `_TransformedKNeighborsRegressor`. All core estimators now either inherit directly from `_TransformedKNeighborsRegressor` or `_KNeighborsRegressor`.
aazuspan
added
refactor
|
Thanks @grovduck, merging! |
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.
This would resolve #51 by:
TransformedKNeighborsRegressorinto an abstract classfitmethods and into an abstract_get_transformermethodfitmethods and into theTransformedKNeighborsRegressor._set_fitted_transformermethod to reduce duplicationYFitMixinto handle transformer fitting forGNNandMSNThis also:
transform_attribute totransformer_for consistency with the new methods_validate_datacheck withforce_all_finite=Truewhen fitting all transformed estimators. This was needed byMSN, but also fixed an xfailing estimator check forGNN, which allowed us to drop that from the tags.@grovduck Sorry about the convoluted commit history... Not sure exactly how, but I accidentally included all of your commits from #50 in this. Once this gets squashed it shouldn't be a problem at least.