Correcting OneClassSVM.fit() Signature by raamana · Pull Request #11933 · scikit-learn/scikit-learn

raamana · 2018-08-28T14:47:16Z

What does this implement/fix? Explain your changes.

The signature of OneClassSVM.fit() method has an unexpected y=None in its signature, although its not being used or documented under Parameters. Moreover kwargs capturing is unncessary as its not documented or expected, and super-class method doesn't take them anyways.

Any other comments?

Perhaps this was a design decision to try match the signatures in BaseLibSVM class, but I don't think this is a good idea.

raamana · 2018-08-28T17:41:02Z

I should have discussed this with the maintainers first before opening the PR, but I was convinced this was a clear mistake that needs to be corrected ;)

Anyways, after looked further into tests, I realize sklearn tests mandate taking y as a second argument for all estimators (check_fit_score_takes_y). As I dig in, I notice pattern appears in other outlier detection methods (one-class to say) as well, such as Elliptic Envelope and Isolation Forest.

I think we should improve the definition for

scikit-learn/sklearn/utils/estimator_checks.py

Lines 1051 to 1075 in 8d7ce8e

    
           def check_fit_score_takes_y(name, estimator_orig): 
        
               # check that all estimators accept an optional y 
        
               # in fit and score so they can be used in pipelines 
        
               rnd = np.random.RandomState(0) 
        
               X = rnd.uniform(size=(10, 3)) 
        
               X = pairwise_estimator_convert_X(X, estimator_orig) 
        
               y = np.arange(10) % 3 
        
               estimator = clone(estimator_orig) 
        
               y = multioutput_estimator_convert_y_2d(estimator, y) 
        
               set_random_state(estimator) 
        
               funcs = ["fit", "score", "partial_fit", "fit_predict", "fit_transform"] 
        
               for func_name in funcs: 
        
                   func = getattr(estimator, func_name, None) 
        
                   if func is not None: 
        
                       func(X, y) 
        
                       args = [p.name for p in signature(func).parameters.values()] 
        
                       if args[0] == "self": 
        
                           # if_delegate_has_method makes methods into functions 
        
                           # with an explicit "self", so need to shift arguments 
        
                           args = args[1:] 
        
                       assert_true(args[1] in ["y", "Y"], 
        
                                   "Expected y or Y as second argument for method " 
        
                                   "%s of %s. Got arguments: %r." 
        
                                   % (func_name, type(estimator).__name__, args))

and remove this check_fit_score_takes_y for one-class or outlier detection methods.

jnothman · 2018-08-28T23:06:49Z

For good or bad we have established this convention, and I see little benefit in changing it.

raamana · 2018-08-28T23:42:34Z

Well, the biggest benefit is to reduce user's confusion of when trying to use .fit() method for one-class or outlier estimators. I was very confused and thrown off today why the fit method for one class sim asking for target labels, and its help didn't clarify it! I had to look into the code to see it not being used, without a comment (e.g. even for fellow developers!). I believe the best way to improve the "integrity of the code" is to add an if condition in the check_fit_score_takes_y test, and clean up the signatures in the y-not-needed methods.

jnothman · 2018-08-29T01:09:51Z

That will break existing meta-estimator code, perhaps inside but certainly outside this resistor. So while we could deprecate the convention, I see little benefit, while this is the convention we establish long ago.

Missing from the OneClassSVM case is the documentation y : ignored seen elsewhere. A pr adding that would be very welcome.

raamana · 2018-08-29T01:45:11Z

I understand your practical concerns - could you elaborate on how you think this will break the meta-estimator code? Also, I am not following you on the "inside" vs "outside" parts you mention. That's likely why I am underestimating how impactful these changes are. I'd be curious to learn more.

while y = ignored or y = do_not_supply_labels might be one way to handle it, it seems really out of place (and of lower-standard) in the sklearn codebase. I'm not trying to make any unnecessary changes, but I believe these changes 1) are not actually so big, and 2) can improve the quality and user experience.

Of course, final call is up to the sklearn dev and maintainer team, and I'd be happy update the PR with that.

cc @amueller @GaelVaroquaux

raamana · 2018-08-29T01:46:38Z

Also, as far as I can check IsolationForest and ElliptivEnvelope are both not doing any better than OneClassSVM:

scikit-learn/sklearn/ensemble/iforest.py

Line 190 in 8d7ce8e

def fit(self, X, y=None, sample_weight=None):

scikit-learn/sklearn/covariance/outlier_detection.py

Line 99 in f0ab589

def fit(self, X, y=None):

jnothman · 2018-08-29T02:21:42Z

Elsewhere in the unsupervised scikit-learn world:

git grep -pi ' y : ig'
sklearn/cluster/affinity_propagation_.py=class AffinityPropagation(BaseEstimator, ClusterMixin):
sklearn/cluster/affinity_propagation_.py:        y : Ignored
sklearn/cluster/bicluster.py=class BaseSpectral(six.with_metaclass(ABCMeta, BaseEstimator,
sklearn/cluster/bicluster.py:        y : Ignored
sklearn/cluster/birch.py=class Birch(BaseEstimator, TransformerMixin, ClusterMixin):
sklearn/cluster/birch.py:        y : Ignored
sklearn/cluster/birch.py:        y : Ignored
sklearn/cluster/dbscan_.py=class DBSCAN(BaseEstimator, ClusterMixin):
sklearn/cluster/dbscan_.py:        y : Ignored
sklearn/cluster/dbscan_.py:        y : Ignored
sklearn/cluster/hierarchical.py=class AgglomerativeClustering(BaseEstimator, ClusterMixin):
sklearn/cluster/hierarchical.py:        y : Ignored
sklearn/cluster/hierarchical.py=class FeatureAgglomeration(AgglomerativeClustering, AgglomerationTransform):
sklearn/cluster/hierarchical.py:        y : Ignored
sklearn/cluster/k_means_.py=class KMeans(BaseEstimator, ClusterMixin, TransformerMixin):
sklearn/cluster/k_means_.py:        y : Ignored
sklearn/cluster/k_means_.py:        y : Ignored
sklearn/cluster/k_means_.py:        y : Ignored
sklearn/cluster/k_means_.py:        y : Ignored
sklearn/cluster/k_means_.py=class MiniBatchKMeans(KMeans):
sklearn/cluster/k_means_.py:        y : Ignored
sklearn/cluster/k_means_.py:        y : Ignored
sklearn/cluster/mean_shift_.py=class MeanShift(BaseEstimator, ClusterMixin):
sklearn/cluster/mean_shift_.py:        y : Ignored
sklearn/cluster/optics_.py=class OPTICS(BaseEstimator, ClusterMixin):
sklearn/cluster/optics_.py:        y : ignored
sklearn/cluster/spectral.py=class SpectralClustering(BaseEstimator, ClusterMixin):
sklearn/cluster/spectral.py:        y : Ignored
sklearn/decomposition/dict_learning.py=class SparseCoder(BaseEstimator, SparseCodingMixin):
sklearn/decomposition/dict_learning.py:        y : Ignored
sklearn/decomposition/dict_learning.py=class DictionaryLearning(BaseEstimator, SparseCodingMixin):
sklearn/decomposition/dict_learning.py:        y : Ignored
sklearn/decomposition/dict_learning.py=class MiniBatchDictionaryLearning(BaseEstimator, SparseCodingMixin):
sklearn/decomposition/dict_learning.py:        y : Ignored
sklearn/decomposition/dict_learning.py:        y : Ignored
sklearn/decomposition/factor_analysis.py=class FactorAnalysis(BaseEstimator, TransformerMixin):
sklearn/decomposition/factor_analysis.py:        y : Ignored
sklearn/decomposition/factor_analysis.py:        y : Ignored
sklearn/decomposition/fastica_.py=class FastICA(BaseEstimator, TransformerMixin):
sklearn/decomposition/fastica_.py:        y : Ignored
sklearn/decomposition/fastica_.py:        y : Ignored
sklearn/decomposition/incremental_pca.py=class IncrementalPCA(_BasePCA):
sklearn/decomposition/incremental_pca.py:        y : Ignored
sklearn/decomposition/incremental_pca.py:        y : Ignored
sklearn/decomposition/nmf.py=class NMF(BaseEstimator, TransformerMixin):
sklearn/decomposition/nmf.py:        y : Ignored
sklearn/decomposition/nmf.py:        y : Ignored
sklearn/decomposition/online_lda.py=class LatentDirichletAllocation(BaseEstimator, TransformerMixin):
sklearn/decomposition/online_lda.py:        y : Ignored
sklearn/decomposition/online_lda.py:        y : Ignored
sklearn/decomposition/online_lda.py:        y : Ignored
sklearn/decomposition/pca.py=class PCA(_BasePCA):
sklearn/decomposition/pca.py:        y : Ignored
sklearn/decomposition/pca.py:        y : Ignored
sklearn/decomposition/pca.py:        y : Ignored
sklearn/decomposition/sparse_pca.py=class SparsePCA(BaseEstimator, TransformerMixin):
sklearn/decomposition/sparse_pca.py:        y : Ignored
sklearn/decomposition/sparse_pca.py=class MiniBatchSparsePCA(SparsePCA):
sklearn/decomposition/sparse_pca.py:        y : Ignored
sklearn/decomposition/truncated_svd.py=class TruncatedSVD(BaseEstimator, TransformerMixin):
sklearn/decomposition/truncated_svd.py:        y : Ignored
sklearn/decomposition/truncated_svd.py:        y : Ignored
sklearn/manifold/isomap.py=class Isomap(BaseEstimator, TransformerMixin):
sklearn/manifold/isomap.py:        y : Ignored
sklearn/manifold/isomap.py:        y : Ignored
sklearn/manifold/locally_linear.py=class LocallyLinearEmbedding(BaseEstimator, TransformerMixin):
sklearn/manifold/locally_linear.py:        y : Ignored
sklearn/manifold/locally_linear.py:        y : Ignored
sklearn/manifold/mds.py=class MDS(BaseEstimator):
sklearn/manifold/mds.py:        y : Ignored
sklearn/manifold/mds.py:        y : Ignored
sklearn/manifold/t_sne.py=class TSNE(BaseEstimator):
sklearn/manifold/t_sne.py:        y : Ignored
sklearn/manifold/t_sne.py:        y : Ignored
sklearn/preprocessing/_discretization.py=class KBinsDiscretizer(BaseEstimator, TransformerMixin):
sklearn/preprocessing/_discretization.py:        y : ignored
sklearn/preprocessing/data.py=class PowerTransformer(BaseEstimator, TransformerMixin):
sklearn/preprocessing/data.py:        y : Ignored

$ git grep -A1 '        y$'
sklearn/covariance/empirical_covariance_.py=class EmpiricalCovariance(BaseEstimator):
--
sklearn/covariance/empirical_covariance_.py:        y
sklearn/covariance/empirical_covariance_.py-            not used, present for API consistence purpose.
--
sklearn/covariance/empirical_covariance_.py:        y
sklearn/covariance/empirical_covariance_.py-            not used, present for API consistence purpose.
--
sklearn/covariance/robust_covariance.py=class MinCovDet(EmpiricalCovariance):
--
sklearn/covariance/robust_covariance.py:        y
sklearn/covariance/robust_covariance.py-            not used, present for API consistence purpose.
--
sklearn/covariance/shrunk_covariance_.py=class ShrunkCovariance(EmpiricalCovariance):
--
sklearn/covariance/shrunk_covariance_.py:        y
sklearn/covariance/shrunk_covariance_.py-            not used, present for API consistence purpose.
--
sklearn/covariance/shrunk_covariance_.py=class LedoitWolf(EmpiricalCovariance):
--
sklearn/covariance/shrunk_covariance_.py:        y
sklearn/covariance/shrunk_covariance_.py-            not used, present for API consistence purpose.
--
sklearn/covariance/shrunk_covariance_.py=class OAS(EmpiricalCovariance):
--
sklearn/covariance/shrunk_covariance_.py:        y
sklearn/covariance/shrunk_covariance_.py-            not used, present for API consistence purpose.
--
sklearn/preprocessing/data.py=class MinMaxScaler(BaseEstimator, TransformerMixin):
--
sklearn/preprocessing/data.py:        y
sklearn/preprocessing/data.py-            Ignored
--
sklearn/preprocessing/data.py=class StandardScaler(BaseEstimator, TransformerMixin):
--
sklearn/preprocessing/data.py:        y
sklearn/preprocessing/data.py-            Ignored
--
sklearn/preprocessing/data.py:        y
sklearn/preprocessing/data.py-            Ignored
--
sklearn/preprocessing/data.py=class MaxAbsScaler(BaseEstimator, TransformerMixin):
--
sklearn/preprocessing/data.py:        y
sklearn/preprocessing/data.py-            Ignored
--
sklearn/random_projection.py=class BaseRandomProjection(six.with_metaclass(ABCMeta, BaseEstimator,
--
sklearn/random_projection.py:        y
sklearn/random_projection.py-            Ignored

jnothman · 2018-08-29T02:23:31Z

Meta-estimators naturally call fit on some constituent estimator. If they allow for alternately supervised or unsupervised constituent estimators, they either need to handle the case that the constituent may or may not accept y when fitting it.

We're just not going to change this. Please do submit a PR documenting that y is ignored.

Pradeep Reddy Raamana added 4 commits August 28, 2018 10:38

removing the unnecessary, unexpected and undocumented parameter

e379cbd

removing the unused kwargs capture and passing!

90cd3c9

Clarification on which label is an outlier

5737907

correction to clarification on which label is an outlier

698cce3

jnothman closed this Aug 29, 2018

raamana mentioned this pull request Aug 29, 2018

DOCS: noting y is present in one-class fit methods merely for API consistency #11939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correcting OneClassSVM.fit() Signature#11933

Correcting OneClassSVM.fit() Signature#11933
raamana wants to merge 4 commits intoscikit-learn:masterfrom
raamana:one-class-svm-correct-signature

raamana commented Aug 28, 2018

Uh oh!

raamana commented Aug 28, 2018 •

edited

Loading

Uh oh!

jnothman commented Aug 28, 2018 via email

Uh oh!

raamana commented Aug 28, 2018

Uh oh!

jnothman commented Aug 29, 2018

Uh oh!

raamana commented Aug 29, 2018

Uh oh!

raamana commented Aug 29, 2018

Uh oh!

jnothman commented Aug 29, 2018

Uh oh!

jnothman commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

raamana commented Aug 28, 2018

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

raamana commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman commented Aug 28, 2018 via email

Uh oh!

raamana commented Aug 28, 2018

Uh oh!

jnothman commented Aug 29, 2018

Uh oh!

raamana commented Aug 29, 2018

Uh oh!

raamana commented Aug 29, 2018

Uh oh!

jnothman commented Aug 29, 2018

Uh oh!

jnothman commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

raamana commented Aug 28, 2018 •

edited

Loading