Skip to content
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.

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

FIX use safe_sparse_dot for callable kernel in LabelSpreading (#15866) #15868

Merged
merged 6 commits into from Dec 27, 2019

Conversation

@nik-sm
Copy link
Contributor

nik-sm commented Dec 12, 2019

Fixes #15866

When using a callable kernel function that returns a sparse matrix, the probabilities calculated during LabelSpreading.predict_proba() use np.dot(), which behaves badly when one of the arguments is sparse.

As a follow-up, the kernel that I used as an example here was what I intuitively wanted when I began trying LabelSpreading (RBF weights, but only for a local neighborhood of each data point). It took me some time until I figured out how to calculate this kernel as a callable. Since computing a dense RBF kernel is not feasible for large datasets, this kernel might be useful as a built-in option for other users as well.

If this sounds like a useful addition, I'd be happy to add support and tests for this as a separate PR. For example, this might be provided via kernel='sparse-rbf', 'topk-rbf', 'truncated-rbf' or a similar name.

Copy link
Member

jnothman left a comment

Lgtm! Perhaps consider proposing an example of sparse rbf in another pr. I don't think we want it available by name unless it is well established

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

@nik-sm

This comment has been minimized.

Copy link
Contributor Author

nik-sm commented Dec 12, 2019

Ok. I made 2 minor changes:

  1. I assert issparse on the weight matrix, instead of isinstance(..., csr_matrix). This captures the desired property better, and kneighbors_graph could be changed to return a different sparse format in the future.

  2. I added a check for LabelPropagation as well as LabelSpreading, since both classes were affected by this.

I'm not sure what your preference is for keeping a clean commit history, but if you would like me to squash commits on master branch, please let me know and I'm happy to do that.

I'll follow up later with a separate PR to suggest sparse-rbf kernel.

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Dec 12, 2019

Copy link
Member

thomasjpfan left a comment

Thank you @nik-sm for the PR!

..............................

- |Fix| :class:`semi_supervised.LabelPropagation` and
`semi_supervised.LabelSpreading` now allow callable kernel function to

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Dec 13, 2019

Member

(May require re-wrapping to keep the line width to 79)

Suggested change
`semi_supervised.LabelSpreading` now allow callable kernel function to
:class:`semi_supervised.LabelSpreading` now allow callable kernel function to
n_classes = 4
n_samples = 500
n_test = 10
X, Y = make_classification(n_classes=n_classes,

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Dec 13, 2019

Member

Small style suggestion: (Accuracy is the default scoring for classifiers)

from sklearn.model_selection import train_test_split
...

    X, y = make_classification(n_classes=n_classes,
                               n_samples=n_samples,
                               n_features=20,
                               n_informative=20,
                               n_redundant=0,
                               n_repeated=0,
                               random_state=0)
    X_train, X_test, y_train, y_test = train_test_split(X, y,
                                                        test_size=n_test,
                                                        random_state=0)

    model = label_propagation.LabelSpreading(kernel=topk_rbf)
    model.fit(X_train, y_train)
    assert model.score(X_test, y_test) >= 0.9

    model = label_propagation.LabelPropagation(kernel=topk_rbf)
    model.fit(X_train, y_train)
    assert model.score(X_test, y_test) >= 0.9

This comment has been minimized.

Copy link
@nik-sm

nik-sm Dec 13, 2019

Author Contributor

Great, thanks for the feedback - looks much cleaner

:mod:`sklearn.semi_supervised`
..............................

- |Fix| :class:`semi_supervised.LabelPropagation` and

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Dec 13, 2019

Member

Since this is a bug fix, this should be moved up to Version 0.22.1 in this file.

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Dec 16, 2019

Why do you regard this as a bug fix?

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Dec 16, 2019

I think this should be moved to 0.23 as an enhancement/feature

@nik-sm

This comment has been minimized.

Copy link
Contributor Author

nik-sm commented Dec 16, 2019

If you guys can let me know which category is more appropriate, I'm happy to fix the version tag as needed (or you can also feel free to do it if that is more convenient). My impression at first was that this code was intended to support an arbitrary callable kernel function, in which case it felt like a bugfix (because sparse kernels should have been supported too). However I'm not sure about the original author's intent, so perhaps it's an enhancement instead.

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Dec 17, 2019

It's ambiguous. The more important question is whether it should be included in a patch release or a minor release.

@rth

This comment has been minimized.

Copy link
Member

rth commented Dec 26, 2019

I agree that it's a bit ambiguous on whether it's a bug fix or an enhancement. I'd say let's move it to 0.22.1 section as it's a very minor change and it shouldn't cost too much to include it as a bug fix.

Copy link
Member

qinhanmin2014 left a comment

We handle sparse matrix in fit, so perhaps it's reasonable to also handle it in predict.

I'd say let's move it to 0.22.1 section as it's a very minor change and it shouldn't cost too much to include it as a bug fix.

let's do so

@qinhanmin2014

This comment has been minimized.

Copy link
Member

qinhanmin2014 commented Dec 27, 2019

will merge when green

@qinhanmin2014 qinhanmin2014 added this to the 0.22.1 milestone Dec 27, 2019
@qinhanmin2014 qinhanmin2014 merged commit d163d5a into scikit-learn:master Dec 27, 2019
21 checks passed
21 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 97.45%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +2.54% compared to 74a0874
Details
scikit-learn.scikit-learn Build #20191227.2 succeeded
Details
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
@qinhanmin2014

This comment has been minimized.

Copy link
Member

qinhanmin2014 commented Dec 27, 2019

thanks @nik-sm

ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Dec 31, 2019
ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Jan 2, 2020
ogrisel added a commit that referenced this pull request Jan 2, 2020
* DOC fixed default values in dbscan (#15753)

* DOC fix incorrect branch reference in contributing doc (#15779)

* DOC relabel Feature -> Efficiency in change log (#15770)

* DOC fixed Birch default value (#15780)

* STY Minior change on code padding in website theme (#15768)

* DOC Fix yticklabels order in permutation importances example (#15799)

* Fix yticklabels order in permutation importances example

* STY Update wrapper width (#15793)

* DOC Long sentence was hard to parse and ambiguous in _classification.py (#15769)

* DOC Removed duplicate 'classes_' attribute in Naive Bayes classifiers (#15811)

* BUG Fixes pandas dataframe bug with boolean dtypes (#15797)

* BUG Returns only public estimators in all_estimators (#15380)

* DOC improve doc for multiclass and types_of_target (#15333)

* TST Increases tol for check_pca_float_dtype_preservation assertion (#15775)

* update _alpha_grid class in _coordinate_descent.py (#15835)

* FIX Explicit conversion of ndarray to object dtype. (#15832)

* BLD Parallelize sphinx builds on circle ci (#15745)

* DOC correct url for preprocessing (#15853)

* MNT avoid generating too many cross links in examples (#15844)

* DOC Correct wrong doc in precision_recall_fscore_support (#15833)

* DOC add comment in check_pca_float_dtype_preservation (#15819)

Documenting the changes in #15775

* DOC correct indents in docstring _split.py (#15843)

* DOC fix docstring of KMeans based on sklearn guideline (#15754)

* DOC fix docstring of AgglomerativeClustering based on sklearn guideline (#15764)

* DOC fix docstring of AffinityPropagation based on sklearn guideline (#15777)

* DOC fixed SpectralCoclustering and SpectralBiclustering docstrings following sklearn guideline (#15778)

* DOC fix FeatureAgglomeration and MiniBatchKMeans docstring following sklearn guideline (#15809)

* TST Specify random_state in test_cv_iterable_wrapper (#15829)

* DOC Include LinearSV{C, R} in models that support sample_weights (#15871)

* DOC correct some indents (#15875)

* DOC Fix documentation of default values in tree classes (#15870)

* DOC fix typo in docstring (#15887)

* DOC FIX default value for xticks_rotation in plot_confusion_matrix (#15890)

* Fix imports in pip3 ubuntu by suffixing affected files (#15891)

* MNT Raise erorr when normalize is invalid in confusion_matrix (#15888)

* [MRG] DOC Increases search results for API object results (#15574)

* MNT Ignores warning in pyamg for deprecated scipy.random (#15914)

* DOC Instructions to troubleshoot Windows path length limit (#15916)

* DOC add versionadded directive to some estimators (#15849)

* DOC clarify doc-string of roc_auc_score and add references (#15293)

* MNT Adds skip lint to azure pipeline CI (#15904)

* BLD Fixes bug when building with NO_MATHJAX=1 (#15892)

* [MRG] BUG Checks to number of axes in passed in ax more generically (#15760)

* EXA Minor fixes in plot_sparse_logistic_regression_20newsgroups.py (#15925)

* BUG Do not shadow public functions with deprecated modules (#15846)

* Import sklearn._distributor_init first (#15929)

* DOC Fix typos, via a Levenshtein-style corrector (#15923)

* DOC in canned comment, mention that PR title becomes commit me… (#15935)

* DOC/EXA Correct spelling of "Classification" (#15938)

* BUG fix pip3 ubuntu update by suffixing file (#15928)

* [MRG] Ways to compute center_shift_total were different in "full" and "elkan" algorithms. (#15930)

* TST Fixes integer test for train and test indices (#15941)

* BUG ensure that parallel/sequential give the same permutation importances (#15933)

* Formatting fixes in changelog (#15944)

* MRG FIX: order of values of self.quantiles_ in QuantileTransformer (#15751)

* [MRG] BUG Fixes constrast in plot_confusion_matrix (#15936)

* BUG use zero_division argument in classification_report (#15879)

* DOC change logreg solver in plot_logistic_path (#15927)

* DOC fix whats new ordering (#15961)

* COSMIT use np.iinfo to define the max int32 (#15960)

* DOC Apply numpydoc validation to VotingRegressor methods (#15969)

Co-authored-by: Tiffany R. Williams <Tiffany8@users.noreply.github.com>

* DOC improve naive_bayes.py documentation (#15943)

Co-authored-by: Jigna Panchal <40188288+jigna-panchal@users.noreply.github.com>

* DOC Fix default values in Perceptron documentation (#15965)

* DOC Improve default values in logistic documentation (#15966)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

* DOC Improve documentation of default values for imputers (#15964)

* EXA/MAINT Simplify code in manifold learning example (#15949)

* DOC Improve default values in SGD documentation (#15967)

* DOC Improve defaults in neural network documentation (#15968)

* FIX use safe_sparse_dot for callable kernel in LabelSpreading (#15868)

* BUG Adds attributes back to check_is_fitted (#15947)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* DOC update check_is_fitted what's new

* DOC change python-devel to python3-devel for yum. (#15986)

* DOC Correct the default value of values_format in plot_confusion_matrix (#15981)

* [MRG] MNT Updates pypy to use 7.2.0 (#15954)

* FIX Add missing 'values_format' param to disp.plot() in plot_confusion_matrix (#15937)

* FIX support scalar values in fit_params in SearchCV (#15863)

* support a scalar fit param

* pep8

* TST add test for desired behavior

* FIX introduce _check_fit_params to validate parameters

* DOC update whats new

* TST tests both grid-search and randomize-search

* PEP8

* DOC revert unecessary change

* TST add test for _check_fit_params

* olivier comments

* TST fixes

* DOC whats new

* DOC whats new

* TST revert type of error

* add olivier suggestions

* address olivier comments

* address thomas comments

* PEP8

* comments olivier

* TST fix test by passing X

* avoid to call twice tocsr

* add case column/row sparse in check_fit_param

* provide optional indices

* TST check content when indexing params

* PEP8

* TST update tests to check identity

* stupid fix

* use a distribution in RandomizedSearchCV

* MNT add lightgbm to one of the CI build

* move to another build

* do not install dependencies lightgbm

* MNT comments on the CI setup

* address some comments

* Test fit_params compat without dependency on lightgbm

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* Remove abstractmethod that silently brake downstream packages (#15996)

* FIX restore BaseNB._check_X without abstractmethod decoration (#15997)

* Update v0.22 changelog for 0.22.1 (#16002)

- set the date
- move entry for quantile transformer to the 0.22.1 section
- fix alphabetical ordering of modules

* STY Removes hidden scroll bar (#15999)

* Flake8 fixes

* Fix: remove left-over lines that should have been deleted during conflict resolution when rebasing

* Fix missing imports

* Update version

* Fix test_check_is_fitted

* Make test_sag_regressor_computed_correctly deterministic (#16003)

Fix #15818.

Co-authored-by: cgsavard <claire.savard@colorado.edu>
Co-authored-by: Joel Nothman <joel.nothman@gmail.com>
Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
Co-authored-by: Matt Hall <matt@agilegeoscience.com>
Co-authored-by: Kathryn Poole <kathryn.poole2@gmail.com>
Co-authored-by: lucyleeow <jliu176@gmail.com>
Co-authored-by: JJmistry <jayminm22@gmail.com>
Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>
Co-authored-by: SylvainLan <sylvain.s.lannuzel@gmail.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Hanmin Qin <qinhanmin2005@sina.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Vachan D A <vachanda@users.noreply.github.com>
Co-authored-by: Sambhav Kothari <sambhavs.email@gmail.com>
Co-authored-by: wenliwyan <12013376+wenliwyan@users.noreply.github.com>
Co-authored-by: shivamgargsya <shivam.gargshya@gmail.com>
Co-authored-by: Reshama Shaikh <rs2715@stern.nyu.edu>
Co-authored-by: Oliver Urs Lenz <oulenz@users.noreply.github.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Co-authored-by: Brian Wignall <BrianWignall@gmail.com>
Co-authored-by: Ritchie Ng <ritchieng@u.nus.edu>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: inderjeet <43402782+inder128@users.noreply.github.com>
Co-authored-by: scibol <scibol@users.noreply.github.com>
Co-authored-by: Tirth Patel <tirthasheshpatel@gmail.com>
Co-authored-by: Bibhash Chandra Mitra <bibhashm220896@gmail.com>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Tiffany R. Williams <Tiffany8@users.noreply.github.com>
Co-authored-by: Jigna Panchal <40188288+jigna-panchal@users.noreply.github.com>
Co-authored-by: @nkish <19225359+ankishb@users.noreply.github.com>
Co-authored-by: Pulkit Mehta <pulkit_mehta_work@yahoo.com>
Co-authored-by: David Breuer <DavidBreuer@users.noreply.github.com>
Co-authored-by: Niklas <niklas.sm+github@gmail.com>
Co-authored-by: Windber <guolipengyeah@126.com>
Co-authored-by: Stephen Blystone <29995339+blynotes@users.noreply.github.com>
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.