-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[WIP] ENH estimator FreezeWrap to stop it being cloned/refit #9464
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
Conversation
Again, votes for or against the more magical #8374 are welcome |
tests not passing ;) I generally prefer meta-estimator solutions because they are easy for us and pretty safe and super flexible. @agramfort and @GaelVaroquaux tend not to like them because they feel they are hard to use / inspect. The main use-case of freezing is inside a meta-estimator, so here we're adding another level of indirection. API question: should we freeze the estimator we get if |
I think this is my preferred solution. |
yes, i agree re model inspection: the indirection isn't a problem because
the freezing happens separate from the modelling of the frozen component.
There is no benefit that I can see in deprecating cv=predict in
calibration, but we should certainly deprecate prefit in SelectFromModel.
Being able to tune the selector by grid search is an immediate benefit here
(although it could also be achieved through caching if we didn't assume the
feature importance estimator was pre-built)
On 1 Aug 2017 2:35 am, "Andreas Mueller" <notifications@github.com> wrote:
I think this is my preferred solution.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9464 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz60tx5Y_38HxB4avmhKndZ5dKphPGks5sTgJDgaJpZM4Oni_C>
.
|
Fair. |
sorry, should have been cv='prefit'
…On 2 Aug 2017 7:40 am, "Andreas Mueller" ***@***.***> wrote:
There is no benefit that I can see in deprecating cv=predict in
calibration, but we should certainly deprecate prefit in SelectFromModel.
Fair.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9464 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6wHSmNjMn_9NXpkcajLkkNmfnyYmks5sT5tUgaJpZM4Oni_C>
.
|
I'm now not altogether sure whether freezing is the right solution for transfer learning and semi-supervision... The problem is that you can't search over the parameters of a frozen estimator (to optimise some target supervised prediction accuracy, for instance), something which I assume is desirable. Rather you want some kind of But that doesn't solve the problem of making a pre-trained model appear as part of something. Do we have good examples that motivate freezing??? |
You mean search parameters that would change training? That would be quite different, yes. Is there a reason this implementation doesn't mirror all attributes? Another idea that might be possible would be to create new frozen variants classes based on the estimator we get. That should allow us to cleanly overwrite the |
Oh, yes, this patch doesn't work even for providing coef_ and
feature_importances_ to SelectFromModel. Which either means we need to copy
across the dict, or use __getattr__, or just go back to the simpler #8374.
Frozen variant classes is not a reasonable option as far as I'm concerned.
…On 13 August 2017 at 00:19, Andreas Mueller ***@***.***> wrote:
You mean search parameters that would change training? That would be quite
different, yes.
My motivating examples are still things that have prefit and putting them
in pipelines / cloning them.
Is there a reason this implementation doesn't mirror all attributes?
If we provide a freeze function, we can set all the attributes etc on the
instance match the base estimator, right?
Another idea that might be possible would be to create new frozen variants
classes based on the estimator we get. That should allow us to cleanly
overwrite the fit methods, but also mirror all other functionality, allow
isinstance etc. Also, the class would have a name that's specific to the
estimator but also clearly shows that it's frozen.
Though if we pre-define them we can't freeze 3rd party estimators, and if
we try to create them on the fly, I'm not sure if we could get them to
pickle.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9464 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz64RnjKD8YltgqNUR3DLd3paUOboqks5sXbRtgaJpZM4Oni_C>
.
|
I think if we go with this solution we should copy over the
Can you elaborate? |
no hurry on my part
|
Closing for now |
This is yet another take on freezing, in which we provide a wrapper, unlike #8374 where we simply overwrite key methods. The advantage is that it is less magical and hence less inherently dangerous. But it also makes it much harder to make the frozen object to match all the properties of the contained estimator (e.g.
_estimator_type
, methods, instance checks).TODO:
FreezeWrap
, and mention in freeze and semi-supervised docs