-
-
Notifications
You must be signed in to change notification settings - Fork 33
Sample props specification #6
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
|
Add alternative proposition at the end of the document. |
| |:----------------|:-----------------------------------------------------------------------------------------------| | ||
| | grid.fit | `{'weights': weights, 'cv__groups': cv_groups, split_groups': gs_groups}` | | ||
| | grid.score | `{'weights': weights, 'cv__groups': cv_groups, split_groups': gs_groups}` | | ||
| | grid.split | `{'weights': weights, 'groups': gs_groups, 'cv__groups': cv_groups, split_groups': gs_groups}` | |
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.
grid.split isn't a function..?
jnothman
left a comment
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.
I had originally conceived of the routing idea as telling a meta-estimator where to pass each provided property, i.e. {'cv': ['sample_weight', 'groups']}, etc. But it seems instead we are replicating the data for each routing target. I've not yet thought of problems with this. IMO, it is a move towards deprecating sample_weight, which I think is a reasonable idea (although we should do it slowly, or in version 1.0). And it is somewhat consistent with Pipeline.fit kwargs semantics. I think I like it.
I've not read thoroughly enough to know if these are covered:
- If
sample_propsis not provided, we will not attempt to pass a value for it. - If a
sample_propskey is unused/unwanted by an estimator, should it raise an error (like passingsample_weightwhen unwanted currently does)? If so, this behaviour must be enforced by common tests. - We seem to be creating naming conflicts with this design: we can no longer have a pipeline step named
cvorscore(should it bescoring?) in a grid search. Would we handle that with a warning?
I think this approach is worth trying. But I suggest that we keep the names of keys singular: 'weight', 'group'; not 'weights', 'groups'. In a similar vein, we need to choose the name sample_props, props, prop, etc. carefully.
|
I've just taken a quick look at the Google Doc, not that it's easy to read. I think an approach where routing is handled by meta-estimator is IMO more explicit and resilient to bugs / forwards compatibility issues than when the routing is handled by the base estimator / CV object / etc (akin to dependency injection). It might be easier to write code with the latter style, though. |
|
Another tricky case: sometimes we do if not hasattr(est, 'fit_transform'):
return est.fit(...).transform(). In this case, how do we know if props need
to be passed to transform as well as fit?
…On 8 June 2017 at 02:07, Thierry Guillemot ***@***.***> wrote:
Add alternative proposition at the end of the document.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz61V5PXmz2XQCCFQInuniSGdafoymks5sBsq4gaJpZM4NyvH8>
.
|
|
Yeah the Google doc is not really readable. We stopped in the middle of the
conversation yesterday. What I don't like about routing in meta estimators
is that you could potentially route at many levels, and I tried to
illustrate that. Either you route only on the outermost level and have a
mess of underscores or you need the outer levels to allow for some
pass-through. I feel this is similar to the "attaching search parameters to
estimators" issue that you opened at some point, only here it's worse than
the underscores in param_grid because there is only one grid search in that
case, but here there are arbitrary many levels of meta estimators that
could potentially interact with this.
Sent from phone. Please excuse spelling and brevity.
…On Jun 8, 2017 06:04, "Joel Nothman" ***@***.***> wrote:
Another tricky case: sometimes we do if not hasattr(est, 'fit_transform'):
return est.fit(...).transform(). In this case, how do we know if props need
to be passed to transform as well as fit?
On 8 June 2017 at 02:07, Thierry Guillemot ***@***.***>
wrote:
> Add alternative proposition at the end of the document.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/enhancement_proposals/
pull/6#issuecomment-306843354>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/
AAEz61V5PXmz2XQCCFQInuniSGdafoymks5sBsq4gaJpZM4NyvH8>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAbcFg91Uht4mvZ_mgyqMQHjgXw42Xpaks5sB3LNgaJpZM4NyvH8>
.
|
|
yes, that would be the motivation for specifying routing and data
separately.
It's still not entirely clear to me that a sample_props object is better
than kwargs (except that kwargs currently have different semantics)
…On 8 Jun 2017 6:01 pm, "Andreas Mueller" ***@***.***> wrote:
Yeah the Google doc is not really readable. We stopped in the middle of the
conversation yesterday. What I don't like about routing in meta estimators
is that you could potentially route at many levels, and I tried to
illustrate that. Either you route only on the outermost level and have a
mess of underscores or you need the outer levels to allow for some
pass-through. I feel this is similar to the "attaching search parameters to
estimators" issue that you opened at some point, only here it's worse than
the underscores in param_grid because there is only one grid search in that
case, but here there are arbitrary many levels of meta estimators that
could potentially interact with this.
Sent from phone. Please excuse spelling and brevity.
On Jun 8, 2017 06:04, "Joel Nothman" ***@***.***> wrote:
> Another tricky case: sometimes we do if not hasattr(est,
'fit_transform'):
> return est.fit(...).transform(). In this case, how do we know if props
need
> to be passed to transform as well as fit?
>
> On 8 June 2017 at 02:07, Thierry Guillemot ***@***.***>
> wrote:
>
> > Add alternative proposition at the end of the document.
> >
> > —
> > You are receiving this because you are subscribed to this thread.
> > Reply to this email directly, view it on GitHub
> > <https://github.com/scikit-learn/enhancement_proposals/
> pull/6#issuecomment-306843354>,
> > or mute the thread
> > <https://github.com/notifications/unsubscribe-auth/
> AAEz61V5PXmz2XQCCFQInuniSGdafoymks5sBsq4gaJpZM4NyvH8>
> > .
> >
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/enhancement_proposals/
pull/6#issuecomment-306992398>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAbcFg91Uht4mvZ_
mgyqMQHjgXw42Xpaks5sB3LNgaJpZM4NyvH8>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz60vOvWBJ8jauAoxlsdusna1SjAOAks5sB6p1gaJpZM4NyvH8>
.
|
|
@jnothman It's not great but I tried to summarize the discussion of the google doc |
|
basically because we would need kwargs in all of our fit methods, which is not great and can swallow all kinds of errors. It can also cause problems for depending projects that use fit args. |
|
no i still think we'd only need kwargs in metaestimators (prove me wrong)
and that for base estimators a common test will ensure that they check
kwargs for unwanted props
On 8 Jun 2017 6:58 pm, "Andreas Mueller" <[email protected]> wrote:
basically because we would need kwargs in all of our fit methods, which is
not great and can swallow all kinds of errors. It can also cause problems
for depending projects that use fit args.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz68c4iUNl2mxHTYHzMDKVYJ-amVeEks5sB7eagaJpZM4NyvH8>
.
|
|
hm... and we'd use the same / a similar routing mechanism? I think I like that.... and without routing it's passed to everything? |
|
Well, GridSearch would have default routing replicating current behaviour.
If we're talking about specifying routing at construction rather than in
fit param name, Pipeline will require some change to conform, and is open
to however we choose to route by default.
…On 8 June 2017 at 19:21, Andreas Mueller ***@***.***> wrote:
hm... and we'd use the same / a similar routing mechanism? I think I like
that.... and without routing it's passed to everthing?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz63cSlBMdjFChEx59r7FprdcaaEolks5sB70zgaJpZM4NyvH8>
.
|
|
well pipeline also has a default routing now with fit_params, right? |
|
That's got multiple parameters in fit, so it's got your issue of not having
routing described abstractly as a class param.... but yes.
…On 8 June 2017 at 19:35, Andreas Mueller ***@***.***> wrote:
well pipeline also has a default routing now with fit_params, right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz64UKkECKtiuIYW7DZA9IALhbvRlgks5sB8BbgaJpZM4NyvH8>
.
|
|
We also need to take the |
|
Speaking with @ogrisel IRL, |
|
I don't think we need to handle the case where you want to pass different
weights to different parts of the machinery. Users can invent their own
ways of doing such things. but the ability to turn on/off weighting the
transformer vs the predictor is a more obvious case.
…On 14 June 2017 at 22:15, Guillaume Lemaitre ***@***.***> wrote:
Speaking with @ogrisel <https://github.com/ogrisel> IRL,
TransformTargetRegressor could be considered as use case. It takes a
transformer and a regressor. At fit, one has to choose how to handle the
sample_weight: 2 different sample_weight could be passed for each
estimator, a single for both, etc. For the moment, I could not find a
transformer with a sample_weight but I assume that it could happen to be
useful.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6-batV9ZxfYcxLawZHi1AZtnEDVgks5sD87jgaJpZM4NyvH8>
.
|
|
I've decided this is a/the chief API priority and will try to put some focussed energy into it soon... |
It's be easy to invent one from, say |
|
The document here focuses on the solution without clearly analysing the problem. Our classic case here is
There's some notion raised here that we need a dataframe-like object, but I contend that we've got almost no problems due to sample_weight being a kwarg, except perhaps that:
Advantages of sticking to kwargs include that:
So, IMO the main problem is routing. Will come back with more on that soon. Starting by analysing what we have so far. |
|
I should state the broader problem: |
|
Existing sample props:
Handling of
Principles of routing:
|
|
Further design issues in master:
Questions:
Solution alternatives:
Within 2, there are a few further orthogonal design decisions:
If DIRECTION:=b then there are further questions such as whether routing is specified as a single constructor parameter, or multiple / in a specialised way depending on the meta-estimator. I'll try make these designs, and their benefits, more concrete with example syntaxes at some point soon. |
jnothman
left a comment
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.
So after looking over parts of the proposal here, my main critiques are:
- I don't think we benefit from having
sample_propsas a separate dict fromkwargs. - I don't think we usually benefit from letting an estimator receive a sample property that it does not explicitly support or pass on, so we may not need
seterr - I think we are over-using underscores, when routing paths can use other string delimiters (e.g.
:), or rely on (named)tuples instead.
One challenge which I don't think is addressed so far here, is passing props to predict/transform, if we want to support that case. It's easy to imagine the use of some grouping information for a ranking or a time series problem... although those things could perhaps have been passed in as part of X if need be, or each sample could represent a group. Do we want to support props in predict? This becomes tricky in part because scorers call predict.
|
I solemnly swear I'll catch up to this soon ;) |
|
I'm not sure whether to integrate my comments into the proposal or just
work on a demo
…On 15 Aug 2017 12:06 am, "Andreas Mueller" ***@***.***> wrote:
I solemnly swear I'll catch up to this soon ;)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz69Sefs93nZZoLh5CfP-UWRdt-t0jks5sYFR3gaJpZM4NyvH8>
.
|
|
I think the syntactic elegance of annotating an estimator with what it should receive, but it:
My approach adds a How to specify routing is still open. Let's imagine we want to route parameter
While I knew it was the case for Pipeline, I've belatedly realised that none of these allows us to neatly express the current routing policy for GridSearch, which is "pass everything but groups to estimator.fit; pass groups to cv". One problem I have is forward compatibility if we allow destinations to be matched by patterns rather than exact name matches. If new destination possibilities (such as passing to Question: Cases where we should raise errors:
|
|
I'm off work sick today and seem to have produced a reasonable implementation of a variant at scikit-learn/scikit-learn#9566 |
|
I think it would be great to have a list of application examples and then implement them in each of the alternative proposals. I tried to do that in the google doc, with more and more complex examples, i.e. nested cross-validation, pipelines where sample_weights are passed to some steps, or to score etc. |
|
Yeah, I've been thinking more of the "deep" solution where there is no routing, just kwarg naming conventions. The main problems are that it's verbose (but easy to read) and hard to maintain backwards compatibility, particularly in FeatureUnion and in a small way through making reserved prefixes in cross validation. |
|
Those things aren't issues if we don't use kwargs, but I really think we should stick to kwargs. |
|
And yes, demonstrations are a good idea. I just won't have time for any of this until closer to November, I expect. |
|
I think we can wait another 2 month with this ;) |
I open a PR to let people talk about
sample_props.