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+1] Fixes #10393 Fixed error when fitting RidgeCV with integers #10397

Merged
merged 11 commits into from Mar 8, 2018

Conversation

Projects
None yet
6 participants
@mabelvj
Contributor

mabelvj commented Jan 3, 2018

fixes #10393

  • [Fix bug]: added support for float alphas in class _RidgeGCV(LinearModel), lines 1050 and 1052.
  • [Test]: added a test to check that using negative alphas does not raise an error.

@mabelvj mabelvj changed the title from [WIP] Fixes #10393 Fixed error when fitting RidgeCV with negative alpha to [WIP] Fixes #10393 Fixed error when fitting RidgeCV with negative alphas Jan 3, 2018

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Jan 3, 2018

Member

negative alphas should raise an error, right?

Member

amueller commented Jan 3, 2018

negative alphas should raise an error, right?

@mabelvj mabelvj changed the title from [WIP] Fixes #10393 Fixed error when fitting RidgeCV with negative alphas to [WIP] Fixes #10393 Fixed error when fitting RidgeCV with integers Jan 3, 2018

@mabelvj

This comment has been minimized.

Show comment
Hide comment
@mabelvj

mabelvj Jan 3, 2018

Contributor

Yes! Sorry, I got confused and thought it should not raise an error. It's fixed now, testing negative and positive alphas both integers and float.

Contributor

mabelvj commented Jan 3, 2018

Yes! Sorry, I got confused and thought it should not raise an error. It's fixed now, testing negative and positive alphas both integers and float.

@jnothman

Please use Fixes #10393 in the PR description, rather than something ad hoc like #Fixes issue #10393 so that GitHub knows to close the issue automatically when this is merged.

A first glance:

Show outdated Hide outdated sklearn/linear_model/ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Jan 10, 2018

Contributor

Also I would make the conversion directly from __init__:

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/ridge.py#L886

alphas = np.asarray(alphas, dtype=np.float64)

Contributor

glemaitre commented Jan 10, 2018

Also I would make the conversion directly from __init__:

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/ridge.py#L886

alphas = np.asarray(alphas, dtype=np.float64)

@mabelvj

This comment has been minimized.

Show comment
Hide comment
@mabelvj

mabelvj Jan 30, 2018

Contributor

Hi! Is it ok now?

Contributor

mabelvj commented Jan 30, 2018

Hi! Is it ok now?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jan 30, 2018

Member

If you think the work is complete, please change WIP in the title to MRG

Member

jnothman commented Jan 30, 2018

If you think the work is complete, please change WIP in the title to MRG

Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated

@mabelvj mabelvj changed the title from [WIP] Fixes #10393 Fixed error when fitting RidgeCV with integers to [MRG] Fixes #10393 Fixed error when fitting RidgeCV with integers Jan 30, 2018

Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
Show outdated Hide outdated sklearn/linear_model/ridge.py Outdated
@jnothman

flake8 error.

Otherwise LGTM

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jan 31, 2018

Member

Please add an entry to the change log under Bug Fixes 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:

Member

jnothman commented Jan 31, 2018

Please add an entry to the change log under Bug Fixes 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 from [MRG] Fixes #10393 Fixed error when fitting RidgeCV with integers to [MRG+1] Fixes #10393 Fixed error when fitting RidgeCV with integers Feb 8, 2018

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
@mabelvj

This comment has been minimized.

Show comment
Hide comment
@mabelvj

mabelvj Feb 15, 2018

Contributor

I'm sorry for all the mess, I'm new to open source and did not know how to deal with remote changes and do the pull --rebase, that's why some the parts got removed. I updated the documentation adding my line and then reverted again the issue with lambdas.

Contributor

mabelvj commented Feb 15, 2018

I'm sorry for all the mess, I'm new to open source and did not know how to deal with remote changes and do the pull --rebase, that's why some the parts got removed. I updated the documentation adding my line and then reverted again the issue with lambdas.

@qinhanmin2014

LGTM overall, @mabelvj please try to avoid unrelevant changes (there's still some extra blank lines). Also, please try to fill current line before starting a new line.

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
@@ -419,4 +423,4 @@ Changes to estimator checks
- Add test :func:`estimator_checks.check_methods_subset_invariance` to check
that estimators methods are invariant if applied to a data subset.
:issue:`10420` by :user:`Jonathan Ohayon <Johayon>`

This comment has been minimized.

@qinhanmin2014

qinhanmin2014 Mar 3, 2018

Member

Try to get rid of this strange diff.

@qinhanmin2014

qinhanmin2014 Mar 3, 2018

Member

Try to get rid of this strange diff.

This comment has been minimized.

@mabelvj

mabelvj Mar 8, 2018

Contributor

I don't know, the file in the master has already that line. Should I remove it?

@mabelvj

mabelvj Mar 8, 2018

Contributor

I don't know, the file in the master has already that line. Should I remove it?

This comment has been minimized.

@qinhanmin2014

qinhanmin2014 Mar 8, 2018

Member

If you actually don't change anything and find it hard to get rid of it, you might just keep it. (Hope there won't be some strange things when merging)

@qinhanmin2014

qinhanmin2014 Mar 8, 2018

Member

If you actually don't change anything and find it hard to get rid of it, you might just keep it. (Hope there won't be some strange things when merging)

@@ -1090,7 +1096,7 @@ def __init__(self, alphas=(0.1, 1.0, 10.0),
fit_intercept=True, normalize=False, scoring=None,
cv=None, gcv_mode=None,
store_cv_values=False):
self.alphas = alphas
self.alphas = np.asarray(alphas)

This comment has been minimized.

@qinhanmin2014

qinhanmin2014 Mar 3, 2018

Member

Why doing so?

@qinhanmin2014

qinhanmin2014 Mar 3, 2018

Member

Why doing so?

This comment has been minimized.

@mabelvj

mabelvj Mar 8, 2018

Contributor

I was suggested to make that changes a few lines above: to add a conversion in the __init__ and then remove the float. It's done in the init of _RidgeGCV.

@mabelvj

mabelvj Mar 8, 2018

Contributor

I was suggested to make that changes a few lines above: to add a conversion in the __init__ and then remove the float. It's done in the init of _RidgeGCV.

Show outdated Hide outdated sklearn/linear_model/tests/test_ridge.py Outdated
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Mar 8, 2018

Member

@mabelvj Thanks for the explanation. I don't think I'll focus too much on these minor things, so please:
(1) resolved the conflict
(2) avoid all unrelevant changes (please double check your diff here)
((3) better try to fill current line before starting a new line and remove some unnecessary blank line)
I think it's very close from being merged.

Member

qinhanmin2014 commented Mar 8, 2018

@mabelvj Thanks for the explanation. I don't think I'll focus too much on these minor things, so please:
(1) resolved the conflict
(2) avoid all unrelevant changes (please double check your diff here)
((3) better try to fill current line before starting a new line and remove some unnecessary blank line)
I think it's very close from being merged.

@qinhanmin2014

LGTM. I've pushed some minor change about the format.

@qinhanmin2014 qinhanmin2014 merged commit 03dd287 into scikit-learn:master Mar 8, 2018

0 of 5 checks passed

ci/circleci: python2 CircleCI is running your tests
Details
ci/circleci: python3 CircleCI is running your tests
Details
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 Running analyses for revisions
Details
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014
Member

qinhanmin2014 commented Mar 8, 2018

Thanks @mabelvj :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment