Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Add additional validation for user-supplied gram matrixes #19004

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 11 commits into from
Jan 5, 2021
Merged

Add additional validation for user-supplied gram matrixes #19004

merged 11 commits into from
Jan 5, 2021

Conversation

amidvidy
Copy link
Contributor

Reference Issues/PRs

#18973

What does this implement/fix? Explain your changes.

  1. Adds additional validation check for precomputed gram matrix suggested by @agramfort.
  2. Adds tests for the above check,
  3. Adds a new example illustrating how to correctly compute gram matrix when sample weights are also supplied.

Any other comments?

Thanks in advance for reviewing.

agramfort
agramfort reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

thx @amidvidy

examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
sklearn/linear_model/tests/test_coordinate_descent.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
@amidvidy
Copy link
Contributor Author

i believe I have resolved the above comments @agramfort

glemaitre
glemaitre reviewed Dec 16, 2020
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

I did a first pass.

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

examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
@amidvidy amidvidy mentioned this pull request Dec 19, 2020
Closed
@amidvidy
Copy link
Contributor Author

thanks for the review @glemaitre. I believe I have resolved your comments, with the exception the calls to .copy() in the unit test, for which I have filed #19044.

@amidvidy
Copy link
Contributor Author

#19044 was resolved, so will update test and rebase.

glemaitre
glemaitre reviewed Jan 4, 2021
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

A couple of last changes

examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
examples/linear_model/plot_elastic_net_precomputed_gram_matrix_with_weighted_samples.py Outdated Show resolved Hide resolved
sklearn/linear_model/_base.py Outdated
X : ndarray of shape (n_samples, n_features)
Data array.

precompute: array-like of shape (n_features, n_features)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
precompute: array-like of shape (n_features, n_features)
precompute : array-like of shape (n_features, n_features)

Copy link
Member

Choose a reason for hiding this comment

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

I assume that precompute should be an ndarray and not an array-like as specified in the docstring.

Copy link
Contributor Author

@amidvidy amidvidy Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

in the docstring for the ElasticNet constructor, the precompute param is also specified as array-like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/_coordinate_descent.py#L611

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've kept it as array-like for consistency, perhaps it should be updated in both places?

sklearn/linear_model/_base.py Show resolved Hide resolved
sklearn/linear_model/tests/test_coordinate_descent.py Show resolved Hide resolved
@amidvidy
Copy link
Contributor Author

amidvidy commented Jan 5, 2021

the test failures on this latest commit seem to be related to #19096.

@glemaitre
Copy link
Member

the test failures on this latest commit seem to be related to #19096.

Yes it should be fixed pretty soon. We can just discard it.

@glemaitre glemaitre merged commit 2d95acf into scikit-learn:master Jan 5, 2021
@glemaitre
Copy link
Member

Thanks @amidvidy. Good to be merged.

@amidvidy
Copy link
Contributor Author

amidvidy commented Jan 5, 2021

Thanks again for the help with this PR @glemaitre & @agramfort

@cmarmo cmarmo mentioned this pull request Jan 18, 2021
Closed
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
@MalteKurz MalteKurz mentioned this pull request Sep 29, 2021
Closed
This was referenced Dec 16, 2021
Closed
Closed
@MalteKurz MalteKurz mentioned this pull request Dec 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@agramfort agramfort agramfort approved these changes

Assignees
No one assigned
Labels
module:linear_model module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amidvidy @glemaitre @agramfort
Morty Proxy This is a proxified and sanitized view of the page, visit original site.