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

[MRG+2] Add parameter to linear_kernel for dense_output #10999

Merged
merged 6 commits into from Apr 25, 2018

Conversation

Projects
None yet
3 participants
@tgsmith61591
Copy link
Contributor

tgsmith61591 commented Apr 19, 2018

Reference Issues/PRs

This PR references #10980.

What does this implement/fix? Explain your changes.

This adds an optional dense_output=True parameter to the metrics.pairwise.linear_kernel, similar to that in metrics.pairwise.cosine_similarity. I've also added the parameter into the docstring, updated the versionadded, and written a unit test that asserts the output is sparse and equal to the expected dense output.

@tgsmith61591 tgsmith61591 changed the title [WIP] Add parameter to linear_kernel for dense_output [MRG] Add parameter to linear_kernel for dense_output Apr 19, 2018

@rth
Copy link
Member

rth left a comment

Thanks for your PR @tgsmith61591 !

Please find some comments below

@@ -540,6 +540,18 @@ def test_linear_kernel():
assert_array_almost_equal(K.flat[::6], [linalg.norm(x) ** 2 for x in X])


def test_linear_kernel_sparse_output():

This comment has been minimized.

@rth

rth Apr 21, 2018

Member

This is very redundant with test_cosine_similarity_sparse_output.
Let's just make this latter test a parametric test with pytest, that takes as input the similarity function (and the metric name), to check both for cosine similarity and the linear kernel. Also possibly rename that test to something more generic.

This comment has been minimized.

@tgsmith61591

tgsmith61591 Apr 23, 2018

Author Contributor

No problem. I fixed this in the latest push.

This comment has been minimized.

@rth

rth Apr 23, 2018

Member

No what I meant is to define a single function and make it parametric with pytest,

import pytest

@pytest.mark.parametrized('name, pairwise_func',
                          [('linear_kernel', linear_kernel),
                           ('cosine_similarity', cosine_similarity)])
def test_pairwise_similarity_sparse_output(name, pairwise_func):
    # adapt the previous version of test_cosine_similarity_sparse_output here

see https://docs.pytest.org/en/latest/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions

``False``, the output is sparse if both input arrays are sparse.
.. versionadded:: 0.20
parameter ``dense_output`` for dense output.

This comment has been minimized.

@rth

rth Apr 21, 2018

Member

Please remove this line; versionadded + the version alone is sufficient.

@rth

rth approved these changes Apr 24, 2018

Copy link
Member

rth left a comment

Thanks!

@rth rth changed the title [MRG] Add parameter to linear_kernel for dense_output [MRG+1] Add parameter to linear_kernel for dense_output Apr 24, 2018

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Apr 25, 2018

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

@jnothman jnothman changed the title [MRG+1] Add parameter to linear_kernel for dense_output [MRG+2] Add parameter to linear_kernel for dense_output Apr 25, 2018

@tgsmith61591

This comment has been minimized.

Copy link
Contributor Author

tgsmith61591 commented Apr 25, 2018

@jnothman Done. Thanks!

@jnothman jnothman merged commit 9eb3568 into scikit-learn:master Apr 25, 2018

0 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
lgtm analysis: Python Fetching git commits
Details
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Apr 25, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.