Repository: scikit-learn/scikit-learn
Pull request: EHN raise error with inconsistant transformer_weight in FeatureUnion
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #17863.

#### 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`.

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
FeatureUnion silently ignores unknown transformer weights
<!--
Before submitting a bug, please make sure the issue hasn't been already
addressed by searching through the past issues.
-->

#### Describe the bug
<!--
A clear and concise description of what the bug is.
-->
Entries in the `transformer_weights` parameter of `FeatureUnion` which do not correspond to a transformer in the union object are silently ignored. I argue that it would be beneficial if a warning would be shown instead, as this can be a very tedious problem to debug if a typo is in the transformer name.

I don't see any valid reasons why one would deliberately provide excess entries in the weights parameter and counts on them being ignored by the `FeatureUnion`.

For the other way around, I don't see a problem, not providing a weight for a transformer results in not scaling it, which seems intuitive and appropriate.

#### Steps/Code to Reproduce

```python
from sklearn.pipeline import FeatureUnion
from sklearn.feature_extraction.text import CountVectorizer

weights = {
    'words': 1.0,
    'cahrs': 0.5,  # has a typo
}

features = FeatureUnion([
    ('words', CountVectorizer(analyzer='word')),
    ('chars', CountVectorizer(analyzer='char')),
], transformer_weights=weights)   # produces no warning
```

#### Expected Results
<!-- Example: No error is thrown. Please paste or describe the expected results.-->
A warning telling the user that a weight was provided for a transformer which is not in the FeatureUnion.

#### Actual Results
<!-- Please paste or specifically describe the actual output or traceback. -->
Silence

#### Versions
<!--
Please run the following snippet and paste the output below.
For scikit-learn >= 0.20:
import sklearn; sklearn.show_versions()
For scikit-learn < 0.20:
import platform; print(platform.platform())
import sys; print("Python", sys.version)
import numpy; print("NumPy", numpy.__version__)
import scipy; print("SciPy", scipy.__version__)
import sklearn; print("Scikit-Learn", sklearn.__version__)
import imblearn; print("Imbalanced-Learn", imblearn.__version__)
-->
```
System:
    python: 3.7.5 (default, Dec  4 2019, 15:51:49)  [GCC 9.2.0]
executable: /home/benjamin/.cache/pypoetry/virtualenvs/cltc-8COAUnlf-py3.7/bin/python
   machine: Linux-5.7.6-arch1-1-x86_64-with-arch

Python dependencies:
          pip: 20.1.1
   setuptools: 41.2.0
      sklearn: 0.23.1
        numpy: 1.19.0
        scipy: 1.4.1
       Cython: 0.29.14
       pandas: 1.0.5
   matplotlib: 3.2.2
       joblib: 0.16.0
threadpoolctl: 2.1.0

Built with OpenMP: True
```

<!-- Thanks for contributing! -->

Hints from issue discussion:
Thanks for the report. A fix would be welcome
@jnothman I was just looking at FeatureUnion. I'll put together a quick fix and open a PR later today.

Commit message: Add comment to test

Patch to review:
diff --git a/sklearn/pipeline.py b/sklearn/pipeline.py
index 992f63ab77703..ebecb3017a8e2 100644
--- a/sklearn/pipeline.py
+++ b/sklearn/pipeline.py
@@ -13,6 +13,7 @@
 from itertools import islice
 
 import numpy as np
+import warnings
 from scipy import sparse
 from joblib import Parallel, delayed
 
@@ -868,6 +869,17 @@ def _validate_transformers(self):
                                 "transform. '%s' (type %s) doesn't" %
                                 (t, type(t)))
 
+    def _validate_transformer_weights(self):
+        if self.transformer_weights:
+            transformer_names = set(name for name, _ in self.transformer_list)
+            for name in self.transformer_weights:
+                if name not in transformer_names:
+                    warnings.warn(
+                        'Attempting to weight transformer "{0}", but it is '
+                        'not present in transformer_list.'.format(name),
+                        UserWarning
+                    )
+
     def _iter(self):
         """
         Generate (name, trans, weight) tuples excluding None and
@@ -957,6 +969,7 @@ def _parallel_func(self, X, y, fit_params, func):
         """Runs func in parallel on X and y"""
         self.transformer_list = list(self.transformer_list)
         self._validate_transformers()
+        self._validate_transformer_weights()
         transformers = list(self._iter())
 
         return Parallel(n_jobs=self.n_jobs)(delayed(func)(
diff --git a/sklearn/tests/test_pipeline.py b/sklearn/tests/test_pipeline.py
index 3f838d044a1e1..c4466628b314e 100644
--- a/sklearn/tests/test_pipeline.py
+++ b/sklearn/tests/test_pipeline.py
@@ -1220,3 +1220,19 @@ def transform(self, X, y=None):
 
     t.fit(X, y, a=0)
     t.fit_transform(X, y, a=0)
+
+
+def test_feature_union_warns_unknown_transformer_weight():
+    # Warn user when transformer_weights containers a key not present in
+    # transformer_list
+    X = [[1, 2], [3, 4], [5, 6]]
+    y = [0, 1, 2]
+
+    transformer_list = [('transf', Transf())]
+    # Transformer weights dictionary with incorrect name
+    weights = {'transformer': 1}
+    expected_msg = ('Attempting to weight transformer "transformer", '
+                    'but it is not present in transformer_list.')
+    fu = FeatureUnion(transformer_list, transformer_weights=weights)
+    with pytest.warns(UserWarning, match=expected_msg):
+        fu.fit(X, y)


Code review comments:
diff_hunk: @@ -868,6 +869,17 @@ def _validate_transformers(self):
                                 "transform. '%s' (type %s) doesn't" %
                                 (t, type(t)))
 
+    def _validate_transformer_weights(self):
+        if self.transformer_weights:
path: sklearn/pipeline.py
text: @user1:
Nit: I have a preference to early exiting, which reduces the levels of indentation:

```py
if not self.transformer_weights:
	return

transformer_names = ...
```



@author:
Doesn't seem like a nit to me, I'm a big fan is fewer indentation levels.