nice 👍 +1 for merge

LGTM

Since it's only deprecation, would it suit to put in 0.14 and shorten the
cycle? (I know it's not a bug fix.)

On Wed, Jul 31, 2013 at 7:36 PM, Alexandre Gramfort <
notifications@github.com> wrote:

LGTM

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2334#issuecomment-21850543
.

And there is also a backwards-compatibility issue with check_cv, which is
apparently part of the public API, as it had returned a generator with
indices=False for non-sparse data (undocumented). Does the change in
behaviour need to be documented?

On Wed, Jul 31, 2013 at 7:43 PM, Joel Nothman
jnothman@student.usyd.edu.auwrote:

Since it's only deprecation, would it suit to put in 0.14 and shorten the
cycle? (I know it's not a bug fix.)

On Wed, Jul 31, 2013 at 7:36 PM, Alexandre Gramfort <
notifications@github.com> wrote:

LGTM

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2334#issuecomment-21850543
.

it should not break people's code so I would just add a note in the what's
new API section.

I get the following test failures:

======================================================================
ERROR: sklearn.tests.test_common.test_regressors_train
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.5/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/ogrisel/code/scikit-learn/sklearn/tests/test_common.py", line 798, in test_regressors_train
    assert_raises(ValueError, regressor.fit, X, y[:-1])
  File "/usr/local/Cellar/python/2.7.5/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py", line 475, in assertRaises
    callableObj(*args, **kwargs)
  File "/Users/ogrisel/code/scikit-learn/sklearn/linear_model/least_angle.py", line 937, in fit
    for train, test in cv)
  File "/Users/ogrisel/code/scikit-learn/sklearn/externals/joblib/parallel.py", line 516, in __call__
    for function, args, kwargs in iterable:
  File "/Users/ogrisel/code/scikit-learn/sklearn/linear_model/least_angle.py", line 937, in <genexpr>
    for train, test in cv)
IndexError: index 199 is out of bounds for size 199

----------------------------------------------------------------------

I don't get that on master. So it seems to have an impact there. @agramfort or @fabianp can you reproduce?

Since it's only deprecation, would it suit to put in 0.14 and shorten the cycle?

I am not very confortable with this: the change has side effects. Let us
wait a bit.

I am not very confortable with this: the change has side effects. Let us
wait a bit.

np

On Thu, Aug 1, 2013 at 6:12 AM, Gael Varoquaux notifications@github.comwrote:

Since it's only deprecation, would it suit to put in 0.14 and shorten
the cycle?

I am not very confortable with this: the change has side effects. Let us
wait a bit.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2334#issuecomment-21891942
.

I cannot reproduce this failure. Anyone else?

problem reproduced using Canopy.

can you add a :

X, y = check_arrays(X, y)

at the beginning of the fit in LarsCV and OrthogonalMatchingPursuitCV ?

it should fix the problem. Don't ask me why it's not necessary on
my other python setup...

@GaelVaroquaux, I've attempted that check_cv warning. Atm, however, they'll get two warnings:

sklearn/cross_validation.py:1222: DeprecationWarning: check_cv will return indices instead of boolean masks from 0.17
  'masks from 0.17', DeprecationWarning)
sklearn/cross_validation.py:63: DeprecationWarning: The indices parameter is deprecated and will be removed (assumed True) in 0.17
  "removed (assumed True) in 0.17", DeprecationWarning)
sklearn.cross_validation.KFold(n=3, n_folds=3)

Perhaps I should either pass a parameter to affect the KFold warnings, or use warnings module to filter them out...

@ogrisel, I've heeded @agramfort to add some check_arrays. Fixed for you?

That works for me as well. +1 for merge. Thanks @agramfort for the fix.

👍 for merge!

Seeing as @GaelVaroquaux was concerned about check_cv, I'd like to hear from him before merge.

@GaelVaroquaux, is this good to merge? I just want to check that the deprecation of check_cv satisfactory.

I have rebased this on master, and intend to merge tomorrow unless @GaelVaroquaux objects.

I just merged #2370 to clean up the doc a bit. I think this PR should further edit the documentation (after rebasing onto master) to not show the indices=True option anymore.

I meant "to not show the indices=False option anymore".

Merged by rebase, docs fixed.

Thanks!

Hmmm... I've realised that despite the support for this PR, it's not complete: the DeprecationWarning will only show if indices=False, not if indices is explicitly passed as True. I'll throw together a fix.