-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement independent predict
and score
methods for all estimators
#50
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aazuspan, definitely in draft state pending your ideas. Lots of drama for what amounts to five lines of code to make the independent predict
and score
work correctly 😉.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @grovduck!
I mentioned in the issue that I slightly prefer the fit attributes to independent methods, but seeing this in practice, I could go either way! Maybe the question should be whether you'd ever call score_independent
with a different y
than was used to fit the model? If so, a method definitely makes more sense.
Lots of drama for what amounts to five lines of code to make the independent predict and score work correctly 😉.
Nothing's ever as simple as it should be!
@aazuspan, I can't think of an instance where we'd need to call
Is this what you were thinking? I think we'd have to introduce a similar |
That was what I was thinking, but having to duplicate it for Alternatively, we could 1) keep Any thoughts on those options or other ideas? |
Yep, good call! Option 2 makes me uneasy partly because of the mixed functionality that you suggest (and partly because I can't come up with a succinct name that would incorporate both sets of functionality!). The only other option that I can think of (that doesn't really add anything) is to directly subclass So absent those solutions, I think my default is to keep it in the |
I like this suggestion! For one thing, it will simplify the inheritance a little bit, and for another I've been starting to feel uneasy about the fact that we have mixins that all of our estimators are using, which seems to sort of defeat the composability benefit of mixins. I haven't fully thought through the MRO and whether everything will get called in the correct order, but if that all works out then this has my vote. Unfortunately, I'm going to throw a wrench in the works by asking whether we should also combine |
Totally a fair question. I'll run with this and get As far as naming this subclass, what do you think of making it a "private" class (e.g. |
* 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
I think I like that idea! Using an abstract base class would be another way to make it clear that it shouldn't be instantiated directly, but that has some other implications that I don't think we want to use, like requiring subclasses to implement their own versions of methods. Going with a private class seems like a good straightforward solution (and avoids the challenge of coming up with a unique name!). |
I think we're bumping into some MRO issues as you thought might happen. Here's a draft
For the estimators, we can choose the order of the superclasses, but let's assume that we list
When we call If we reverse the order of the superclases in the estimators (e.g. I'm hoping you have a typical brilliant solution!! |
This is a tricky one! I definitely don't have any quick fixes coming to mind, so in the interest of trying to think through the problem out loud... I would say that There may be a really clever solution for reworking our inheritance that solves all these problems, but I think I'm stumped at the moment. Assuming you haven't had any breakthroughs on this, I'm happy to go with the |
@aazuspan, I'll throw this one out there, but I think I'm going to call you the "master architect" here, so you can tell me if you find this distasteful. Instead of using
So then
Thoughts? Too deep of a class hierarchy? I know it goes against the mix-in philosophy of |
I'm totally open to this design! It does seem less idiomatic to the
|
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`.
Excellent! I'll go ahead and create a commit for your review. I've made other biggish changes below and thought it would be easier to just get a commit in there.
This is a great question! RF-NN won't be able to use KNeighborsRegressor (different way of finding distance), but probably would benefit from functionality in (what was)
I think I really like this design because
Yep, initially I had
I totally agree. I took the lazy way out and have renamed to
Yep, I agree here as well. Implemented in the commit. |
This is great! I think you're 100% right about splitting out the kNN-independent functionality, and this is a really clean and clear way to go about it. I do feel a little weird that Overall, I think this is a big improvement to readability and gives us a lot more flexibility going forward. Great work!
I'm happy with this, but I do have an alternative idea for us to consider. I think we discussed earlier that it's a little odd that |
Yes, you're absolutely right about this. I've edited my comment above to reflect this. This doesn't preclude other classes that use this mixin from implement their own crosswalk, but I agree that it may not have a lot of utility outside of kNN estimators. Still, as you note, I think it's a compromise we can live with.
Perfect! Great idea and I think that's an intuitive way to handle this. I've pushed another commit with this change. I decided to remove |
Yeah, I think you're right - this solution looks great to me. Nice to get rid of a module that wasn't adding much! As far as I'm concerned, this looks ready to merge! |
@aazuspan, not that it's a huge deal, but I always manage to screw up the commit message when I merge things back in, so this one is a bit messy, but I didn't want to try to amend it. Hope it doesn't cause too much confusion. |
No problem, it looks clear enough to me! And if anyone needs more details, they can always find the linked discussion here. |
The
IndependentPredictionMixin
class provides two methods (predict_independent
andscore_independent
) that don't include the query point itself when finding nearest neighbors. These methods are similar in functionality to callingKNeighborsRegressor.kneighbors()
without passingX
. In this way, only "independent" neighbors are used in calculating predictions and scores.EDIT: This PR evolved into addressing some class hierarchy issues with our current mixins and inheritance from scikit-learn's
KNeighborsRegressor
. Read through the PR for details on what has changed.This PR will fix #45.