[MRG + 1] ENH add check_inverse in FunctionTransformer #9399

glemaitre · 2017-07-18T13:17:30Z

Reference Issue

We mentioned in #9041 that it could be nice to port the check_inverse inside the FunctionTransformer

What does this implement/fix? Explain your changes.

Make the checking in the fit methods.

Any other comments?

jnothman

Just some thoughts on when this might be too restrictive

jnothman · 2017-07-18T23:26:36Z

sklearn/preprocessing/_function_transformer.py

@@ -59,23 +62,60 @@ class FunctionTransformer(BaseEstimator, TransformerMixin):

        .. deprecated::0.19

+    check_inverse : bool, (default=False)
+       Whether to check that ``transform`` followed by ``inverse_transform``
+       or ``func`` followed by ``inverse_func`` leads to the original targets.


Targets -> input

jnothman · 2017-07-18T23:28:06Z

sklearn/preprocessing/_function_transformer.py

+                                            size=n_subsample,
+                                            replace=False)
+        if sparse.issparse(X):
+            X_sel = X[subsample_idx].A


I don't think you can be sure that the function accepts dense data, so you can't do this.

Also, use safe_indexing to support passing lists or DataFrames through.

jnothman · 2017-07-18T23:30:27Z

sklearn/preprocessing/_function_transformer.py

+    def _validate_inverse(self, X):
+        """Check that func and inverse_func are the inverse."""
+        # Apply the transform and inverse_transform on few samples.
+        X = check_array(X, accept_sparse='csr')


This makes too many assumptions to be practical. Then again allclose below will convert to array (but not to a particular shape or sparse format, etc)

glemaitre · 2017-07-19T14:14:47Z

@jnothman I used the assert_allclose_dense_sparse which look particularly ugly. Is it good enough or we should move the base code into utils.allclose_dense_sparse?

Also, it seems that safe_indexing do not handle COO matrix. Is it worth mentioning in the docstring?

jnothman · 2017-07-19T21:52:45Z

ahh right. sure, it's worth mentioning.

amueller · 2017-07-21T16:04:16Z

sklearn/preprocessing/_function_transformer.py

+
+    def _validate_inverse(self, X):
+        """Check that func and inverse_func are the inverse."""
+        random_state = check_random_state(self.random_state)


utils.resample?

amueller

Looks good apart from prints and possible use of resample.

amueller · 2017-07-21T16:04:24Z

sklearn/preprocessing/_function_transformer.py

+                                            replace=False)
+
+        X_sel = safe_indexing(X, subsample_idx)
+        print(subsample_idx)


amueller · 2017-07-21T16:04:37Z

sklearn/preprocessing/tests/test_function_transformer.py

+              sparse.csc_matrix(X_dense)]
+
+    for X in X_list:
+        print(X)


amueller · 2017-07-21T18:16:05Z

LGTM apart from pep8 break

jorisvandenbossche

Added some comments.

One further thing I was wondering whether the random_state kwarg is worth adding. It's used by check_inverse, so if you for some reason need to control it, I agree you need this keyword, but it just feels a bit like adding 'noise' to the class for most cases.

jorisvandenbossche · 2017-07-26T15:52:35Z

sklearn/preprocessing/_function_transformer.py

@@ -59,23 +60,54 @@ class FunctionTransformer(BaseEstimator, TransformerMixin):

        .. deprecated::0.19

+    check_inverse : bool, (default=False)
+       Whether to check that ``transform`` followed by ``inverse_transform``
+       or ``func`` followed by ``inverse_func`` leads to the original inputs.


mentioning both transform/inverse_transform and func/inverse_func is a leftover from copying from the other PR?
As here you only have func and inverse_func as kwargs.

jorisvandenbossche · 2017-07-26T15:53:03Z

sklearn/preprocessing/_function_transformer.py

@@ -59,23 +60,54 @@ class FunctionTransformer(BaseEstimator, TransformerMixin):

        .. deprecated::0.19

+    check_inverse : bool, (default=False)


spurious comma ?

On line 23 above (can't put a comment there), it is stated that "A FunctionTransformer will not do any checks on its function's output.", which is still correct for the default, but might get an update to mention check_inverse ?

jorisvandenbossche · 2017-07-26T16:07:24Z

sklearn/preprocessing/_function_transformer.py

+        try:
+            assert_allclose_dense_sparse(
+                X_sel, self.inverse_transform(self.transform(X_sel)),
+                atol=1e-7)


why not the default?

jorisvandenbossche · 2017-07-26T16:10:02Z

sklearn/preprocessing/_function_transformer.py

+        If RandomState instance, random_state is the random number generator;
+        If None, the random number generator is the RandomState instance used
+        by np.random. Note that this is used to compute if func and
+        inverse_func are the inverse of each other.


I would clarify that this is only done for check_inverse=True

glemaitre · 2017-07-27T09:34:39Z

@amueller @jnothman what are you thoughts regarding the random_state.
I have the same concerns that @jorisvandenbossche so I would need advice on the way to go.

jnothman · 2017-07-27T10:22:35Z

Drop random_state. Make it deterministic. An evenly-space slice X[::max(1, X.shape[0] // 100)],; perhaps (good because it avoids copying the array, bad because in some cases it will only take a prefix of the data)?

jnothman · 2017-07-27T10:24:11Z

My main concern with this PR is that users aren't going to use the feature if it's not the default. Yes, we might internally. Should we make it eventually be default behaviour? But doing so also complicates what's meant to be a simple utility.

glemaitre · 2017-07-27T13:24:56Z

@jnothman If we make it default it will not be back-compatible, isn't (we might need to deprecate the current behaviour). As a default it would make sense since the majority should have this behaviour.

Drop random_state. Make it deterministic.

It makes sense to me.

bad because in some cases it will only take a prefix of the data)

Not sure to know what you mean by "take a prefix of the data". If this is what I am thinking of, I am not sure that random sampling will make it more robust.

amueller · 2017-07-27T16:04:10Z

👍 for making it deterministic.

amueller · 2017-07-27T16:04:54Z

test failure related.

codecov · 2017-07-31T14:06:50Z

Codecov Report

Merging #9399 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9399      +/-   ##
==========================================
+ Coverage   96.19%   96.19%   +<.01%     
==========================================
  Files         335      335              
  Lines       61800    61825      +25     
==========================================
+ Hits        59448    59473      +25     
  Misses       2352     2352

Impacted Files	Coverage Δ
sklearn/preprocessing/_function_transformer.py	`97.95% <100%> (+0.66%)`	⬆️
...n/preprocessing/tests/test_function_transformer.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02a31f0...f3c0d10. Read the comment docs.

glemaitre · 2017-07-31T17:44:53Z

@jnothman do you see any other changes?

jnothman · 2017-08-01T05:39:31Z

sklearn/preprocessing/_function_transformer.py

@@ -59,6 +58,12 @@ class FunctionTransformer(BaseEstimator, TransformerMixin):

        .. deprecated::0.19

+    check_inverse : bool, default=False
+       Whether to check that or ``func`` followed by ``inverse_func`` leads to
+       the original inputs.


I think this is weak on motivation. Are we trying to recommend it basically as a sanity check / quality assurance?

Sanity check seems a good motivation

amueller · 2017-08-02T15:35:08Z

sklearn/preprocessing/tests/test_function_transformer.py

+                                    inverse_func=np.log1p,
+                                    accept_sparse=accept_sparse,
+                                    check_inverse=True)
+        Xt = trans.fit_transform(X)


assert_no_warning?

amueller · 2017-08-02T15:35:46Z

doc/modules/preprocessing.rst

@@ -610,6 +610,9 @@ a transformer that applies a log transformation in a pipeline, do::
    array([[ 0.        ,  0.69314718],
           [ 1.09861229,  1.38629436]])

+We can ensure that ``func`` and ``inverse_func`` are the inverse of each other


I would say "You" but doesn't matter ;)

amueller · 2017-08-02T15:36:25Z

LGTM (still) apart from checking no warning is raised in the good case.

glemaitre · 2017-08-17T13:50:22Z

@jnothman Could you put this PR in your revision queue?

jnothman · 2017-08-17T13:56:02Z

sklearn/preprocessing/_function_transformer.py

@@ -92,7 +121,9 @@ def fit(self, X, y=None):
        self
        """
        if self.validate:
-            check_array(X, self.accept_sparse)
+            X = check_array(X, self.accept_sparse)
+        if self.check_inverse:


and self.inverse_func is not None

jnothman · 2017-08-17T13:56:58Z

sklearn/preprocessing/_function_transformer.py

+                          " inverse of each other. If you are sure you"
+                          " want to proceed regardless, set"
+                          " 'check_inverse=False'. This warning will turn to"
+                          " an error from 0.22", DeprecationWarning)


It should not be a DeprecationWarning. It's a UserWarning, I think, or perhaps a RuntimeWarning.

We could just allow the user to use a warning filter to make it raise an error (particularly if we define a new warning class), rather than changing behaviour in two versions. I think that's okay in the transformed target case too...

After all, raising an error because the inverse transform was not perfect seems a bit harsh.

I am not against keeping a warning

jnothman · 2017-08-17T13:59:11Z

Believe it or note the current test suite doesn't appear to include a case of actually fitting a FunctionTransformer where inverse_func is not provided.

jnothman

Otherwise LGTM

jnothman · 2017-08-17T21:40:54Z

sklearn/preprocessing/_function_transformer.py

+       .. versionadded:: 0.20
+
+       .. deprecated:: 0.20
+          ``check_inverse=True`` is currently raising a warning if the


Please update. Definitely don't use deprecated tag

jnothman · 2017-08-17T21:41:38Z

doc/modules/preprocessing.rst

@@ -610,6 +610,9 @@ a transformer that applies a log transformation in a pipeline, do::
    array([[ 0.        ,  0.69314718],
           [ 1.09861229,  1.38629436]])

+You can ensure that ``func`` and ``inverse_func`` are the inverse of each other


Please note that a warning is raised and can be turned into an error with: give simplefilter example which tests message

glemaitre · 2017-08-21T12:46:02Z

@jnothman so it should be ready to be merged this time :)

jnothman

Sorry, these should've been included in the previous batch of comment, but github must've glitched.

jnothman · 2017-08-19T11:13:37Z

doc/modules/preprocessing.rst

+
+  >>> import warnings
+  >>> warnings.filterwarnings("error", message="The provided functions are not"
+  ...                         " strictly inverse of each other", append=True)


why append=True? append=False just means it takes precedence over other filters.

I think we want to make the message check loose enough to allow us to change the message. Perhaps '.*check_inverse.*' but with the UserWarning filter on category, so that if we ever deprecate check_inverse the filter isn't fired. Or else we should just make a new category.

glemaitre · 2017-08-31T12:13:03Z

@jnothman this is corrected

jnothman · 2017-09-01T05:57:21Z

I'm fine with this, but @amueller should confirm his +1 since we changed to always warning, rather than have a deprecated behaviour.

jnothman · 2017-09-01T05:58:41Z

And it's still possible we should make it a custom warning class so filtering is easy. But I'm ambivalent.

…ion_transformer

glemaitre · 2017-10-25T12:31:43Z

@lesteve @amueller Can you give a look to this PR.

amueller · 2017-10-25T20:49:32Z

thanks!

…cs/donigian-update-contribution-guidelines * 'master' of github.com:scikit-learn/scikit-learn: (23 commits) fixes scikit-learn#10031: fix attribute name and shape in documentation (scikit-learn#10033) [MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633 (scikit-learn#10025) [MRG] Fix LogisticRegression see also should include LogisticRegressionCV(scikit-learn#9995) (scikit-learn#10022) [MRG + 1] Labels of clustering should start at 0 or -1 if noise (scikit-learn#10015) MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573) [MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#10005) [MRG + 1] ENH add check_inverse in FunctionTransformer (scikit-learn#9399) [MRG] FIX bug in nested set_params usage (scikit-learn#9999) [MRG+1] Fix LOF and Isolation benchmarks (scikit-learn#9798) [MRG + 1] Fix negative inputs checking in mean_squared_log_error (scikit-learn#9968) DOC Fix typo (scikit-learn#9996) DOC Fix typo: x axis -> y axis (scikit-learn#9985) improve example plot_forest_iris.py (scikit-learn#9989) [MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClustering (scikit-learn#9875) DOC update news DOC Fix three typos in manifold documentation (scikit-learn#9990) DOC add missing dot in docstring DOC Add what's new for 0.19.1 (scikit-learn#9983) Improve readability of outlier detection example. (scikit-learn#9973) DOC: Fixed typo (scikit-learn#9977) ...

…9399) * EHN add check_inverse in FunctionTransformer * Add whats new entry and short narrative doc * Sparse support * better handle sparse data * Address andreas comments * PEP8 * Absolute tolerance default * DOC fix docstring * Remove random state and make check_inverse deterministic * FIX remove random_state from init * PEP8 * DOC motivation for the inverse * make check_inverse=True default with a warning * PEP8 * FIX get back X from check_array * Andread comments * Update whats new * remove blank line * joel s comments * no check if one of forward or inverse not provided * DOC fixes and example of filterwarnings * DOC fix warningfiltering * DOC fix merge error git

glemaitre added 3 commits July 18, 2017 15:15

EHN add check_inverse in FunctionTransformer

4659cb4

Add whats new entry and short narrative doc

72d3c54

Sparse support

df07603

jnothman reviewed Jul 18, 2017

View reviewed changes

better handle sparse data

9a5777c

amueller reviewed Jul 21, 2017

View reviewed changes

Address andreas comments

bd7ad2f

amueller changed the title ~~[MRG] EHN add check_inverse in FunctionTransformer~~ [MRG + 1] EHN add check_inverse in FunctionTransformer Jul 21, 2017

PEP8

5c1851b

jorisvandenbossche reviewed Jul 26, 2017

View reviewed changes

glemaitre added 3 commits July 26, 2017 18:19

Merge branch 'master' into check_inverse_function_transformer

4fd988c

Absolute tolerance default

3a764a7

DOC fix docstring

586e8ca

Remove random state and make check_inverse deterministic

43f876c

FIX remove random_state from init

f3c0d10

PEP8

7a19979

jnothman reviewed Aug 1, 2017

View reviewed changes

DOC motivation for the inverse

e59f493

amueller reviewed Aug 2, 2017

View reviewed changes

glemaitre added 4 commits August 2, 2017 18:01

Andread comments

e4045a1

Merge branch 'master' into check_inverse_function_transformer

c8c23fa

Update whats new

4276618

remove blank line

0297a4a

jnothman reviewed Aug 17, 2017

View reviewed changes

glemaitre added 2 commits August 17, 2017 17:13

joel s comments

677cd2a

no check if one of forward or inverse not provided

cec6f53

jnothman reviewed Aug 17, 2017

View reviewed changes

DOC fixes and example of filterwarnings

5238a33

jnothman reviewed Aug 21, 2017

View reviewed changes

DOC fix warningfiltering

31abd47

jnothman changed the title ~~[MRG + 1] EHN add check_inverse in FunctionTransformer~~ [MRG + 1] ENG add check_inverse in FunctionTransformer Sep 1, 2017

jnothman changed the title ~~[MRG + 1] ENG add check_inverse in FunctionTransformer~~ [MRG + 1] ENH add check_inverse in FunctionTransformer Sep 1, 2017

glemaitre added 2 commits October 25, 2017 14:25

Merge remote-tracking branch 'origin/master' into check_inverse_funct…

4d31e52

…ion_transformer

DOC fix merge error git

65b134a

amueller merged commit 8472350 into scikit-learn:master Oct 25, 2017

		@@ -59,23 +60,54 @@ class FunctionTransformer(BaseEstimator, TransformerMixin):

		.. deprecated::0.19

		check_inverse : bool, (default=False)

[MRG + 1] ENH add check_inverse in FunctionTransformer #9399

[MRG + 1] ENH add check_inverse in FunctionTransformer #9399

Conversation

glemaitre commented Jul 18, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Jul 19, 2017

jnothman commented Jul 19, 2017 via email • edited by amueller Loading

Choose a reason for hiding this comment

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Jul 21, 2017

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Jul 27, 2017

jnothman commented Jul 27, 2017

jnothman commented Jul 27, 2017

glemaitre commented Jul 27, 2017

amueller commented Jul 27, 2017

amueller commented Jul 27, 2017

codecov bot commented Jul 31, 2017

Codecov Report

glemaitre commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Aug 2, 2017

glemaitre commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Aug 17, 2017

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Aug 21, 2017

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glemaitre commented Aug 31, 2017

jnothman commented Sep 1, 2017 • edited Loading

jnothman commented Sep 1, 2017

glemaitre commented Oct 25, 2017

amueller commented Oct 25, 2017

jnothman commented Jul 19, 2017 via email •

edited by amueller

Loading

jnothman commented Sep 1, 2017 •

edited

Loading