Skip to content

[MRG +1] Nystroem params#9166

Merged
ogrisel merged 7 commits intoscikit-learn:masterfrom
amueller:nystroem_params
Jun 30, 2017
Merged

[MRG +1] Nystroem params#9166
ogrisel merged 7 commits intoscikit-learn:masterfrom
amueller:nystroem_params

Conversation

@amueller
Copy link
Copy Markdown
Member

@amueller amueller commented Jun 19, 2017 •
edited by ogrisel
Loading

Follow up on #6145, fixes #5211, #5229.

Currently, gamma, coef0 and degree were not passed to callable kernels. That seems unexpected and prone to cause bugs, so I deprecated that behavior.
This code should work the same in all cases, except the default for the chi2 kernel, which previously errored and now uses gamma-1.

@amueller amueller added this to the 0.19 milestone Jun 19, 2017
@amueller amueller mentioned this pull request Jun 19, 2017
Closed
2 tasks
glemaitre
glemaitre reviewed Jun 20, 2017
View reviewed changes
sklearn/kernel_approximation.py
Ignored by other kernels.

degree : float, default=3
degree : float, default=None
Copy link
Copy Markdown
Member

@glemaitre glemaitre Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Shall mentioned that it has been deprecated using .. deprecated::?

Copy link
Copy Markdown
Member Author

@amueller amueller Jun 20, 2017

Choose a reason for hiding this comment

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

It has not been deprecated. Using it with a callable (in which case it was already being ignored) is deprecated.

Copy link
Copy Markdown
Member

@raghavrv raghavrv Jun 29, 2017

Choose a reason for hiding this comment

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

.. versionchanged:: then?

Copy link
Copy Markdown
Member Author

@amueller amueller Jun 29, 2017

Choose a reason for hiding this comment

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

Changed what? Technically the default changed but not the behavior.
I can say "Changed default value from 3 to None", not sure if that's helpful, though.

MechCoder
MechCoder approved these changes Jun 21, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@MechCoder MechCoder left a comment

Choose a reason for hiding this comment

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

LGTM

@MechCoder MechCoder changed the title Nystroem params [MRG +1] Nystroem params Jun 21, 2017
@vene
Copy link
Copy Markdown
Member

vene commented Jun 26, 2017 •
edited
Loading

needs what's new entry for the bugfix part that changes some default behavior

EDIT 1: I was incorrect, as far as I can tell the default behavior has not changed.

EDIT 2: I was actually half-not-wrong. With default arguments there is no change of behavior, but there is a change when Nystroem(kernel='chi2', gamma != None); previously gamma was incorrectly ignored. This warrants a what's new entry I think.

raghavrv
raghavrv approved these changes Jun 29, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@raghavrv raghavrv left a comment

Choose a reason for hiding this comment

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

This looks good to me, with a whatsnew entry. Though I'm not super familiar with this codebase.

sklearn/kernel_approximation.py
Ignored by other kernels.

degree : float, default=3
degree : float, default=None
Copy link
Copy Markdown
Member

@raghavrv raghavrv Jun 29, 2017

Choose a reason for hiding this comment

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

.. versionchanged:: then?

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Jun 30, 2017

LGTM as well. Squash merging.

@ogrisel ogrisel merged commit 896f9d9 into scikit-learn:master Jun 30, 2017
massich pushed a commit to massich/scikit-learn that referenced this pull request Jul 13, 2017
@amueller
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
e5559bd 
…9166)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@amueller @dmohns
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
c441a7d 
…9166)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@amueller @dmohns
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
ee3ba67 
…9166)
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@amueller @NelleV
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
e181fdc 
…9166)
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@amueller @paulha
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
aa41be7 
…9166)
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@amueller @AishwaryaRK
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
1d751ef 
…9166)
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@amueller @maskani-moh
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
fecba40 
…9166)
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@amueller
Fix handling of Nystroem params with a callable kernel (scikit-learn#…
2fc301d 
…9166)
@rth rth mentioned this pull request Apr 21, 2018
Closed
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

+2 more reviewers

@MechCoder MechCoder MechCoder approved these changes

@raghavrv raghavrv raghavrv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.19

Development

Successfully merging this pull request may close these issues.

7 participants

@amueller @vene @ogrisel @MechCoder @glemaitre @raghavrv @mth4saurabh