8000 RFC try to warn on iid less often by amueller · Pull Request #11613 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

RFC try to warn on iid less often #11613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Aug 22, 2018
Merged

RFC try to warn on iid less often #11613

merged 7 commits into from
Aug 22, 2018

Conversation

amueller
Copy link
Member
@amueller amueller commented Jul 17, 2018 •
edited by rth
Loading

This suppresses the iid warning more often. But then the question is what are good tolerances for allclose?

Fixes #11559

@wdevazelhes
Copy link
Contributor

Fixes #11559

@wdevazelhes
Copy link
Contributor

But then the question is what are good tolerances for allclose?

Do you mean a general discussion on what should be the tolerance in this case (uncertainty on a scoring for CV) or rather running a benchmark on some particular tasks and based on that make a decision ?

@GaelVaroquaux
Copy link
Member

I don't know how to set the tolerance here. I have no idea what the expect difference is.

Have you noticed that a given value gives a strong reduction in warnings?

@qinhanmin2014 qinhanmin2014 added this to the 0.20 milestone Jul 21, 2018
@jnothman
Copy link
Member
jnothman commented Aug 9, 2018

What's the resolution here? We can be conservative and make the tolerance a little smaller, i.e. smaller than anyone tends to report...

@NicolasHug
Copy link
Member
NicolasHug commented Aug 17, 2018 •
edited
Loading

So I made some quick benchmark counting the number of warnings raised by iid in the examples (that's what we're trying to reduce, right?).

When warning everytime when iid='warn' (different from what is done on master now):

~/dev/sklearn(branch:pr/11613*) » python test_iid_warnings.py                                                                                                                      nico@cotier
running examples.applications.plot_face_recognition... Got 1 iid warnings.
running examples.cluster.plot_feature_agglomeration_vs_univariate_selection... Got 2 iid warnings.
running examples.compose.plot_column_transformer_mixed_types... Got 0 iid warnings.
running examples.compose.plot_compare_reduction... Got 2 iid warnings.
running examples.compose.plot_digits_pipe... Got 1 iid warnings.
running examples.compose.plot_feature_union... Got 1 iid warnings.
running examples.covariance.plot_covariance_estimation... Got 1 iid warnings.
running examples.decomposition.plot_pca_vs_fa_model_selection... Got 2 iid warnings.
running examples.exercises.plot_cv_diabetes... Got 1 iid warnings.
running examples.gaussian_process.plot_compare_gpr_krr... Got 1 iid warnings.
running examples.model_selection.grid_search_text_feature_extraction... Got 0 iid warnings.
running examples.model_selection.plot_grid_search_digits... Got 2 iid warnings.
running examples.model_selection.plot_multi_metric_evaluation... Got 1 iid warnings.
running examples.model_selection.plot_nested_cross_validation_iris... Got 150 iid warnings.
running examples.model_selection.plot_randomized_search... Got 2 ii
8000
d warnings.
running examples.neighbors.plot_digits_kde_sampling... Got 1 iid warnings.
running examples.neural_networks.plot_rbm_logistic_classification... Got 0 iid warnings.
running examples.plot_kernel_ridge_regression... Got 2 iid warnings.
running examples.preprocessing.plot_discretization_classification... Got 0 iid warnings.
running examples.svm.plot_rbf_parameters... Got 1 iid warnings.
running examples.svm.plot_svm_scale_c... Got 6 iid warnings.
Total number of iid warnings: 177

With the current changes:

~/dev/sklearn(branch:pr/11613*) » python test_iid_warnings.py                                                                                                                      nico@cotier
running examples.applications.plot_face_recognition... Got 0 iid warnings.
running examples.cluster.plot_feature_agglomeration_vs_univariate_selection... Got 0 iid warnings.
running examples.compose.plot_column_transformer_mixed_types... Got 0 iid warnings.
running examples.compose.plot_compare_reduction... Got 2 iid warnings.
running examples.compose.plot_digits_pipe... Got 0 iid warnings.
running examples.compose.plot_feature_union... Got 1 iid warnings.
running examples.covariance.plot_covariance_estimation... Got 1 iid warnings.
running examples.decomposition.plot_pca_vs_fa_model_selection... Got 0 iid warnings.
running examples.exercises.plot_cv_diabetes... Got 0 iid warnings.
running examples.gaussian_process.plot_compare_gpr_krr... Got 0 iid warnings.
running examples.model_selection.grid_search_text_feature_extraction... Got 0 iid warnings.
running examples.model_selection.plot_grid_search_digits... Got 0 iid warnings.
running examples.model_selection.plot_multi_metric_evaluation... Got 0 iid warnings.
running examples.model_selection.plot_nested_cross_validation_iris... Got 83 iid warnings.
running examples.model_selection.plot_randomized_search... Got 1 iid warnings.
running examples.neighbors.plot_digits_kde_sampling... Got 0 iid warnings.
running examples.neural_networks.plot_rbm_logistic_classification... Got 0 iid warnings.
running examples.plot_kernel_ridge_regression... Got 0 iid warnings.
running examples.preprocessing.plot_discretization_classification... Got 0 iid warnings.
running examples.svm.plot_rbf_parameters... Got 0 iid warnings.
running examples.svm.plot_svm_scale_c... Got 0 iid warnings.
Total number of iid warnings: 88
(sklearn) ------------------------

Looks like most of the warnings come from examples.model_selection.plot_nested_cross_validation_iris

Code:

import importlib
import warnings
import sys

import matplotlib.pyplot as plt


plt.ion()  # interactive mode on, prevents matplotlib from blocking
sys.stderr = open('/dev/null', 'w')  # hide error messages

examples = [  # list of examples directly using ****SearchCV()
    'examples.applications.plot_face_recognition',
    'examples.cluster.plot_feature_agglomeration_vs_univariate_selection',
    'examples.compose.plot_column_transformer_mixed_types',
    'examples.compose.plot_compare_reduction',
    'examples.compose.plot_digits_pipe',
    'examples.compose.plot_feature_union',
    'examples.covariance.plot_covariance_estimation',
    'examples.decomposition.plot_pca_vs_fa_model_selection',
    'examples.exercises.plot_cv_diabetes',
    'examples.gaussian_process.plot_compare_gpr_krr',
    'examples.model_selection.grid_search_text_feature_extraction',
    'examples.model_selection.plot_grid_search_digits',
    'examples.model_selection.plot_multi_metric_evaluation',
    'examples.model_selection.plot_nested_cross_validation_iris',
    'examples.model_selection.plot_randomized_search',
    'examples.neighbors.plot_digits_kde_sampling',
    'examples.neural_networks.plot_rbm_logistic_classification',
    'examples.plot_kernel_ridge_regression',
    'examples.preprocessing.plot_discretization_classification',
    'examples.svm.plot_rbf_parameters',
    'examples.svm.plot_svm_scale_c',
]

n_iid_warnings = 0
for e in examples:
    print('running {}... '.format(e), end='', flush=True)
    with warnings.catch_warnings(record=True) as warnings_:
        # Cause deprecation warnings to always be triggered.
        warnings.simplefilter("always", DeprecationWarning)

        sys.stdout = open('/dev/null', 'w')  # avoid prints from example exec
        importlib.import_module(e)
        sys.stdout = sys.__stdout__  # restore the original stdout.

        iid_warnings = [w for w in warnings_ if
                        "The default of the `iid` parameter will change" in
                        str(w.message)]

        print('Got {} iid warnings.'.format(len(iid_warnings)))
        n_iid_warnings += len(iid_warnings)

print('Total number of iid warnings: {}'.format(n_iid_warnings))

Note: there may be actually more warnings if some of the examples indirectly call one of the CV tools. The list of examples I use comes from git grep --name-only SearchCV examples/

I can try different tol values and see which one has the least number of warnings. I just want to make sure this is actually what we want here.

Edit: Increasing both tol to 1e-3 gives only 1 warning.

NicolasHug
NicolasHug reviewed Aug 17, 2018
View reviewed changes
sklearn/model_selection/_search.py Outdated
if not np.allclose(means_weighted, means_unweighted,
rtol=1e-4, atol=1e-4):
warn = True
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a break?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@qinhanmin2014
Copy link
Member

Is it good to suppresses the deprecation warning? It seems that the common way we handle these problems in this release is to set iid=False in the examples and use @pytest.mark.filterwarnings to filter the warnings in the tests (also recorded in our contributing guide). Will it be an acceptable solution here?

@jnothman
Copy link
Member
jnothman commented Aug 19, 2018 via email

@amueller
Copy link
Member Author

Maybe to add to @jnothman's excellent explanation: Here we're using the examples code as a proxy for the user's code. I want the users code not to give useless warnings.
8000

@amueller
Copy link
Member Author

So if we're conservative we could do 1e-4 or 1e-5 and we'd get rid of some. If we're more aggressive we can do 1e-3 and get rid of most warnings.

@jnothman
Copy link
Member
jnothman commented Aug 20, 2018 via email

@qinhanmin2014
Copy link
Member

I think I'm persuaded here.
So what's our plan for the remaining warnings is the examples? Leave them or add iid=False to them?

@amueller
Copy link
Member Author

I'd leave them, otherwise we need to change again soon.
8000

@amueller
Copy link
Member Author

@jnothman rtol or atol? So I'll just leave it as is?

@amueller
Copy link
Member Author

merge for RC?

@jnothman
Copy link
Member

It would be better to mostly rely on rtol, but I haven't stopped to think about the value.

qinhanmin2014
qinhanmin2014 approved these changes Aug 22, 2018
View reviewed changes
Copy link
Member
@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (maybe we can be more aggressive here).

@qinhanmin2014 qinhanmin2014 merged commit a43e94c into scikit-learn:master Aug 22, 2018
@qinhanmin2014
Copy link
Member

Btw @jnothman @amueller How will you handle existing iid=False in examples?

doc/tutorial/text_analytics/working_with_text_data.rst:  >>> gs_clf = GridSearchCV(text_clf, parameters, cv=5, iid=False, n_jobs=-1)
examples/compose/plot_column_transformer_mixed_types.py:grid_search = GridSearchCV(clf, param_grid, cv=10, iid=False)
examples/preprocessing/plot_discretization_classification.py:                           iid=False)

@amueller
Copy link
Member Author

Well once we deprecate it in 0.22 we'll remove those, right?

@qinhanmin2014
Copy link
Member

Well once we deprecate it in 0.22 we'll remove those, right?

@amueller The issue here is that we don't set iid=False in most examples. Will it be strange for these examples to set iid=False? Should we remove iid=False in these examples?

@amueller
Copy link
Member Author

If it's already there I wouldn't worry too much about it.

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018
@yarikoptic
Merge tag '0.20rc1' into releases
f3aff2d 
* tag '0.20rc1': (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018
@yarikoptic
Merge branch 'releases' into dfsg
610cc46 
* releases: (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018
@yarikoptic
Merge branch 'dfsg' into debian
151b85b 
* dfsg: (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug left review comments

@jnothman jnothman jnothman approved these changes

@qinhanmin2014 qinhanmin2014 qinhanmin2014 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.20
Development

Successfully merging this pull request may close these issues.

deprecation warnings in examples for iid and train_size
6 participants
@amueller @wdevazelhes @GaelVaroquaux @jnothman @NicolasHug @qinhanmin2014
0