explain_weights in Pipelines: minimal version #177
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
Codecov Report
@@ Coverage Diff @@
## master #177 +/- ##
=========================================
- Coverage 97.34% 97.3% -0.05%
=========================================
Files 39 41 +2
Lines 2450 2487 +37
Branches 464 469 +5
=========================================
+ Hits 2385 2420 +35
- Misses 34 35 +1
- Partials 31 32 +1
|
kmike
reviewed
May 1, 2017
eli5/sklearn/transform.py
Outdated
| """ | ||
|
|
||
| import numpy as np # type: ignore | ||
| from sklearn.pipeline import Pipeline |
There was a problem hiding this comment.
it seems # type: ignore should fix mypy build
|
Looks good, thanks for the persistence @jnothman! |
jnothman
commented
May 2, 2017
| @@ -13,3 +13,5 @@ The following functions are exposed to a top level, e.g. | |||
| .. autofunction:: eli5.show_weights | |||
|
|
|||
| .. autofunction:: eli5.show_prediction | |||
|
|
|||
| .. autofunction:: eli5.transform_feature_names | |||
There was a problem hiding this comment.
not certain this belongs here
There was a problem hiding this comment.
Yeah, I was also unsure; do you think this function will be used stand-alone, not as an implementation detail of how to make explain.. / show.. functions work?
|
It's not an implementation detail insofar as users will need to register
their own implementations as they write their own transformers.
That's not to say that it's likely to be called outside of explain_weights.
…On 2 May 2017 at 20:15, Mikhail Korobov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In docs/source/autodocs/eli5.rst
<#177 (comment)>:
> @@ -13,3 +13,5 @@ The following functions are exposed to a top level, e.g.
.. autofunction:: eli5.show_weights
.. autofunction:: eli5.show_prediction
+
+.. autofunction:: eli5.transform_feature_names
Yeah, I was also unsure; do you think this function will be used
stand-alone, not as an implementation detail of how to make explain.. /
show.. functions work?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#177 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAEz671vqj-Q77fv0qjqag7hobJeFpP3ks5r1wJLgaJpZM4NNBx7>
.
|
|
In any case, documentation, of all things, can change later.
…On 2 May 2017 at 20:19, Joel Nothman ***@***.***> wrote:
It's not an implementation detail insofar as users will need to register
their own implementations as they write their own transformers.
That's not to say that it's likely to be called outside of explain_weights.
On 2 May 2017 at 20:15, Mikhail Korobov ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In docs/source/autodocs/eli5.rst
> <#177 (comment)>:
>
> > @@ -13,3 +13,5 @@ The following functions are exposed to a top level, e.g.
> .. autofunction:: eli5.show_weights
>
> .. autofunction:: eli5.show_prediction
> +
> +.. autofunction:: eli5.transform_feature_names
>
> Yeah, I was also unsure; do you think this function will be used
> stand-alone, not as an implementation detail of how to make explain.. /
> show.. functions work?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#177 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz671vqj-Q77fv0qjqag7hobJeFpP3ks5r1wJLgaJpZM4NNBx7>
> .
>
|
kmike
reviewed
May 2, 2017
eli5/sklearn/explain_weights.py
Outdated
| @@ -422,3 +424,17 @@ def _features(target_id): | |||
| method='linear model', | |||
| is_regression=True, | |||
| ) | |||
|
|
|||
|
|
|||
| @explain_weights.register(Pipeline) | |||
There was a problem hiding this comment.
After #170 it should be just @register, so that it is registered both for global and sklearn-specific explain functions.
|
Thanks @jnothman! I'm going to make a new release soon - do you want to add anything else to it? |
|
we'll see how time goes. |
|
thanks for the review and merge! |
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.
It looks like eli5 had a recent release, so it's a pity to come in late for it. But here's a more minimal version of #158, to which more can be added as it is designed and tested.