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] FIX #10540 ElasticNet overwrites X even with copy_X=True #10581

Merged
merged 2 commits into from Feb 13, 2018

Conversation

Projects
None yet
6 participants
@ymazari
Contributor

ymazari commented Feb 3, 2018

Reference Issues/PRs

Fixes #10540. Closes #10568.

What does this implement/fix? Explain your changes.

if copy_X=True:

  • Do not overwrite X
  • Ensure copying happens only once: either during check_input or _pre_fit

Any other comments?

  • Added unit tests for verification.
@jnothman

Otherwise LGTM, thanks

original_X = X.copy()
enet = ElasticNet(copy_X=True)
enet.fit(X, y, check_input=False)

This comment has been minimized.

@jnothman

jnothman Feb 3, 2018

Member

I'd use a loop or pytest.mark.parametrize to vary check_input, to clearly show to the reader what invariant you are trying to uphold.

This comment has been minimized.

@ymazari

ymazari Feb 4, 2018

Contributor

Good suggestion. Added pytest.mark.parametrize.

@jnothman jnothman changed the title from [MRG] FIX #10540 ElasticNet overwrites X even with copy_X=True to [MRG+1] FIX #10540 ElasticNet overwrites X even with copy_X=True Feb 4, 2018

y = check_array(y, order='F', copy=False, dtype=X.dtype.type,
ensure_2d=False)
X, y, X_offset, y_offset, X_scale, precompute, Xy = \
_pre_fit(X, y, None, self.precompute, self.normalize,
self.fit_intercept, copy=False)
self.fit_intercept, copy=not X_copied)

This comment has been minimized.

@agramfort

agramfort Feb 4, 2018

Member

logic is not correct. If you pass check_input=False in fit and copy_X=False in init this code will still make a copy.

This comment has been minimized.

@ymazari

ymazari Feb 4, 2018

Contributor

This bug is trickier than what it initially looked like ;)
I fixed the logic to handle that case.
Thanks a lot for the useful feedback!!

@ymazari

This comment has been minimized.

Contributor

ymazari commented Feb 4, 2018

Aaaah, fix one thing and break 20 others. I am checking/fixing the failures.

@ymazari ymazari changed the title from [MRG+1] FIX #10540 ElasticNet overwrites X even with copy_X=True to [WIP] FIX #10540 ElasticNet overwrites X even with copy_X=True Feb 5, 2018

@ymazari ymazari changed the title from [WIP] FIX #10540 ElasticNet overwrites X even with copy_X=True to [MRG] FIX #10540 ElasticNet overwrites X even with copy_X=True Feb 11, 2018

@ymazari

This comment has been minimized.

Contributor

ymazari commented Feb 11, 2018

@agramfort I made the changes based on your feedback, could you please review it?
@jnothman I switched this PR back to "MRG".

@agramfort

This comment has been minimized.

Member

agramfort commented Feb 11, 2018

LGTM

please update what's new with an entry in the bug fix section.

thanks

@lesteve

This comment has been minimized.

Member

lesteve commented Feb 13, 2018

@ymazari can you add an entry in doc/whats_new/v0.20.rst in the "bug fixes" section as requested by @agramfort? This one can be merged when you do this.

[10540] ElasticNet overwrites X even with copy_X=True
- Set copy=True if copy_X=True
- Ensure copying happens only once
- Fix logic with when both copy_X and check_input are False
- Add unit tests
- Update v0.20.rst
@ymazari

This comment has been minimized.

Contributor

ymazari commented Feb 13, 2018

@lesteve, @agramfort: I updated v0.20.rst.
This is now ready to be merged :)

@@ -234,6 +234,10 @@ Classifiers and regressors
where split threshold could become infinite when values in X were
near infinite. :issue:`10536` by :user:`Jonathan Ohayon <Johayon>`.
- Fixed a bug in :class:`linear_model.ElasticNet` which caused the input
to be overridden even when using parameter ``copy_X=True``.

This comment has been minimized.

@agramfort

agramfort Feb 13, 2018

Member

and check_input=False

@glemaitre

This comment has been minimized.

Contributor

glemaitre commented Feb 13, 2018

by the way it should close #10568 I think

@lesteve

This comment has been minimized.

Member

lesteve commented Feb 13, 2018

I pushed minor tweaks, merging, thanks a lot @ymazari!

@lesteve lesteve merged commit 96a2c10 into scikit-learn:master Feb 13, 2018

0 of 5 checks passed

ci/circleci: python2 Your tests are queued behind your running builds
Details
ci/circleci: python3 Your tests are queued behind your running builds
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
@sklearn-lgtm

This comment has been minimized.

sklearn-lgtm commented Feb 13, 2018

This pull request introduces 37 alerts and fixes 18 - view on lgtm.com

new alerts:

  • 37 for Explicit export is not defined

fixed alerts:

  • 6 for Mismatch between signature and use of an overridden method
  • 3 for Missing call to init during object initialization
  • 3 for Potentially uninitialized local variable
  • 2 for Comparison using is when operands support eq
  • 2 for Wrong number of arguments for format
  • 1 for Conflicting attributes in base classes
  • 1 for Mismatch between signature and use of an overriding method

Comment posted by lgtm.com

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