EHN raise error with inconsistant transformer_weight in FeatureUnion by Ultramann · Pull Request #17876 · scikit-learn/scikit-learn

Ultramann · 2020-07-09T20:35:01Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Passing a transformer_weights dictionary to FeatureUnion with a key that isn't found in transformers_list will cause the transformer to not be weighted. This is the correct, but silent behavior. Would like to warn the user when this happens.

This PR adds new a new method _validate_transformer_weights, called at end of FeatureUnion.__init__. This method loops through all the names in transformer_weights, raising a warning if it is not a name in the transformer_list.

Ultramann · 2020-07-13T16:31:22Z

It looks like the two failed checks might be related to work in #17913. I changed the status of the PR to WIP until @thomasjpfan's fix gets merged and I can run again.

jnothman

Do you think this should be warning or error? Are there cases where users may have exploited this behaviour intentionally?

Otherwise LGTM

Ultramann · 2020-07-16T03:25:37Z

I think that's a really good question, it was at the back of my mind too, so I'm glad you asked it. I'm struggling to think of a way that this could be exploited. I'm also inclined to think of it from the opposite perspective, is not throwing an error violating the contract that FeatureUnion exposes. I don't think this is the case, FeatureUnion say's it will weight a transformer by name, "Keys are transformer names" is in the doc string. If you pass the wrong name then the transformer will/should not be weighted. While this points to throwing a warning, it isn't quite enough of a reason to choose not throwing an error. If you tried to look up the wrong key in a dictionary, you'd get a key error. So maybe what we want is behavior similar to that. I'm curious if you, @jnothman, or anyone else has an opinion.

Ultramann · 2020-07-26T17:12:15Z

@jnothman, having thought about your question a bit more I'm thinking that a warning is the right way to go. The way the FeatureUnion's doc string is written makes me think that there's no reason to expect an error if a transformer is referenced by the wrong name in the weights argument.

thomasjpfan

Thank you for the PR @Ultramann !

Can we place the check in fit? I would prefer new code to follow our own guidelines and validate in fit.

Ultramann · 2020-07-26T17:29:22Z

@thomasjpfan, I can definitely move the validation to fit. Should I also move _validate_transformers while I'm at it? Maybe not since that's out of the scope of this PR?

Also, regarding @jnothman's question above, are you good with a warning?

Do you think this should be warning or error?

Ultramann · 2020-07-26T17:49:39Z

@thomasjpfan looking again I found _validate_transformers in the _parallel_func method. So I moved _validate_transformer_weights there too.

thomasjpfan · 2020-07-26T18:04:14Z

Should I also move _validate_transformers while I'm at it?

For this PR, let's leave _validate_transformers alone (in __init__)

…mer_weights

thomasjpfan

Please add an entry to the change log at doc/whats_new/v0.24.rst with tag |Enhancement|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

thomasjpfan

LGTM!

Ultramann · 2020-08-17T01:10:04Z

@jnothman could you take another look at this?

glemaitre · 2020-08-21T17:26:49Z

I would advocate raising an error because nobody reading either the warning or the documentation :)
More seriously, the docstring mentioned that Keys are transformer names. We could add that if the key is not a correct name then we will raise an error. I don't see the use-case where it would be meaningful to pass an inappropriate (or unknown) name.

glemaitre · 2020-08-21T17:27:14Z

@thomasjpfan did you think about any usecase?

glemaitre

Otherwise, I am fine with the implementation

glemaitre · 2020-08-21T17:34:16Z

+        if not self.transformer_weights:
+            return
+
+        transformer_names = set(name for name, _ in self.transformer_list)


Second thought: I find it weird to make a set here. We should not have two transformers with the same name, isn't it?

I agree that we should not have two transformers with the same name. But since transformer_names is being used to solely to determine membership with if name not in transformer_names on line 878, I thought a set would be more appropriate. Happy to change it to a list if that seems more intuitive.

Thinking a bit more about it, it will be more efficient with the in operator afterwards. So let's use the set.

thomasjpfan · 2020-08-22T02:17:23Z

@thomasjpfan did you think about any usecase?

I agree raising an error would be better.

Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

Co-authored-by: Thomas J. Fan <[email protected]>

glemaitre

I will merge when it is green

glemaitre · 2020-08-25T09:51:25Z

Thanks @Ultramann

…cikit-learn#17876) Co-authored-by: Cary Goltermann <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

First pass at adding transformer weight name validation

214f7b1

Ultramann marked this pull request as draft July 9, 2020 20:35

github-actions Bot added the module:pipeline label Jul 9, 2020

Add test for _validate_transformer_weights

9ddc024

Ultramann changed the title ~~[WIP] Validate transformer_weight names in FeatureUnion~~ [MRG] Validate transformer_weight names in FeatureUnion Jul 10, 2020

Ultramann marked this pull request as ready for review July 10, 2020 13:48

Ultramann added 2 commits July 13, 2020 10:02

Update testing warning capture to pytest.warns

ac17f5d

Fix pep8 line length issue

ba28b7f

Ultramann changed the title ~~[MRG] Validate transformer_weight names in FeatureUnion~~ [WIP] Validate transformer_weight names in FeatureUnion Jul 13, 2020

Merge sklearn:master

97e49ce

Ultramann changed the title ~~[WIP] Validate transformer_weight names in FeatureUnion~~ [MRG] Validate transformer_weight names in FeatureUnion Jul 15, 2020

jnothman reviewed Jul 16, 2020

View reviewed changes

thomasjpfan reviewed Jul 26, 2020

View reviewed changes

Ultramann added 2 commits July 26, 2020 12:38

Move transformer weight validation to fit method

a84f87c

Move transformer weight validation to _parallel_func

2822105

Ultramann added 3 commits July 26, 2020 13:58

Merge remote-tracking branch 'upstream/master' into warn_unk_transfor…

224389c

…mer_weights

Update test to expect warning at fit time

0aac742

Add comment to test

b3dba67

Ultramann requested review from jnothman and thomasjpfan July 26, 2020 20:05

thomasjpfan reviewed Jul 26, 2020

View reviewed changes

Comment thread sklearn/pipeline.py Outdated

Ultramann added 2 commits July 26, 2020 15:27

Exit early from transformer weight validation if there are none

d46a0aa

Update change log

4651fe8

thomasjpfan approved these changes Jul 27, 2020

View reviewed changes

glemaitre reviewed Aug 21, 2020

View reviewed changes

Comment thread sklearn/pipeline.py Outdated

glemaitre reviewed Aug 21, 2020

View reviewed changes

Comment thread sklearn/pipeline.py Outdated

glemaitre reviewed Aug 21, 2020

View reviewed changes

Ultramann added 4 commits August 22, 2020 14:20

Merge upstream/master warn_unk_transformer_weights

5e7ec54

Convert user warning into key error, update test

12b7c47

Add note to doc string about raising key error

40f13b4

Update change log

7c4d50c

thomasjpfan reviewed Aug 23, 2020

View reviewed changes

Comment thread sklearn/pipeline.py Outdated

glemaitre reviewed Aug 24, 2020

View reviewed changes

Comment thread sklearn/pipeline.py Outdated

Comment thread doc/whats_new/v0.24.rst Outdated

Comment thread sklearn/tests/test_pipeline.py Outdated

Apply suggestions from code review, key -> value error

e07a9be

Co-authored-by: Guillaume Lemaitre <[email protected]> Co-authored-by: Thomas J. Fan <[email protected]>

thomasjpfan approved these changes Aug 25, 2020

View reviewed changes

Comment thread sklearn/tests/test_pipeline.py Outdated

Update sklearn/tests/test_pipeline.py

3325211

Co-authored-by: Thomas J. Fan <[email protected]>

glemaitre changed the title ~~[MRG] Validate transformer_weight names in FeatureUnion~~ EHN raise error with inconsistant transformer_weight in FeatureUnion Aug 25, 2020

glemaitre approved these changes Aug 25, 2020

View reviewed changes

glemaitre merged commit a0a76fc into scikit-learn:master Aug 25, 2020

Conversation

Ultramann commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

Ultramann commented Jul 13, 2020

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Ultramann commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ultramann commented Jul 26, 2020

Uh oh!

thomasjpfan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ultramann commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ultramann commented Jul 26, 2020

Uh oh!

thomasjpfan commented Jul 26, 2020

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Ultramann commented Aug 17, 2020

Uh oh!

glemaitre commented Aug 21, 2020

Uh oh!

glemaitre commented Aug 21, 2020

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glemaitre Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

Ultramann Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Aug 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ultramann commented Jul 9, 2020 •

edited

Loading

Ultramann commented Jul 16, 2020 •

edited

Loading

thomasjpfan left a comment •

edited

Loading

Ultramann commented Jul 26, 2020 •

edited

Loading