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] FIX consistency of memory layout for linear CD solver #5337

Merged
merged 2 commits into from Oct 5, 2015

Conversation

Projects
None yet
5 participants
@ogrisel
Member

ogrisel commented Oct 2, 2015

This should fix #5013.

This PR fixes several related consistency issues in the handling of the memory layout in models using the linear coordinate descent solvers (Gram or not).

Also I made the expectation w.r.t. memory layout explicit in the Cython prototypes directly which should prevent re-introducing regressions in the future.

@arthurmensch I would appreciate a review on this as I touched a lot of code you changed when introducing the ability to skip input checks. Especially if you have your benchmark scripts at hand it would be great if you could check that I do not re-introduce unwanted redundant input checks.

@ogrisel ogrisel added this to the 0.17 milestone Oct 2, 2015

@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 2, 2015

BTW, I still get a memory error reported by valgrind when I use openblas from ubuntu 14.04 but I think this is in openblas it-self and it does not seem to cause an error at our level. There might be a bug in openblas but I think this PR fixes the issue reported in #5013.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 2, 2015

Travis revealed that the contiguity / ordering depends on the version of numpy. I am on it.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 2, 2015

Should be fixed. My last commit might reveal a bug in graph lasso with old versions of numpy...

@agramfort

This comment has been minimized.

Member

agramfort commented Oct 4, 2015

LGTM

@arthurmensch are you ok ?

@arthurmensch

This comment has been minimized.

Contributor

arthurmensch commented Oct 4, 2015

I ll review and bench it this evening
On Oct 4, 2015 4:44 PM, "Alexandre Gramfort" notifications@github.com
wrote:

LGTM

@arthurmensch https://github.com/arthurmensch are you ok ?


Reply to this email directly or view it on GitHub
#5337 (comment)
.

@ogrisel ogrisel changed the title from [MRG] FIX consistency of memory layout for linear CD solver to [MRG+1] FIX consistency of memory layout for linear CD solver Oct 4, 2015

check_input : bool, default True
Skip input validation checks, including the Gram matrix when provided
assuming there are handled by the caller when check_input=False.

This comment has been minimized.

@arthurmensch

arthurmensch Oct 5, 2015

Contributor

Ok so I guess it is okay to present this flag to the user ?

This comment has been minimized.

@ogrisel

ogrisel Oct 5, 2015

Member

Yes I think so: check_input flags are already present in several other functions / methods in the scikit-learn code base.

@@ -389,11 +392,10 @@ def enet_path(X, y, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
# X should be normalized and fit already if function is called
# from ElasticNet.fit
if pre_fit:

This comment has been minimized.

@arthurmensch

arthurmensch Oct 5, 2015

Contributor

The pre_fit flag was used so that enet_path does not run _pre_fit a second time when called from ElasticNet.fit.

In my experience _pre_fit can take a long time as it forces recomputation of gram matrix when noticing that a change on X has been performed during preprocessing (centering, normalization).

This comment has been minimized.

@ogrisel

ogrisel Oct 5, 2015

Member

But I observed that pre_fit=False whenever check_input is False hence I decided to collapse the two flags together. Did I miss something?

This comment has been minimized.

@arthurmensch

arthurmensch Oct 5, 2015

Contributor

This is true, in fact enet_path is always called with check_input = False from ElasticNet.fit, as necessary checks has been performed already. I once thought that using two flags would be more explicit, but merging them is indeed cleaner.

@arthurmensch

This comment has been minimized.

Contributor

arthurmensch commented Oct 5, 2015

I see no performance regression on dictionary learning, so 👍

Looking back on _pre_fit, I feel that this line is not very clean

    if hasattr(precompute, '__array__') and (
            fit_intercept and not np.allclose(X_mean, np.zeros(n_features))
            or normalize and not np.allclose(X_std, np.ones(n_features))):

despite being important for performance. But that is another issue.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 5, 2015

Great, thanks for the reviews merging. I will not update what's new as this issue was not present in 0.16.

ogrisel added a commit that referenced this pull request Oct 5, 2015

Merge pull request #5337 from ogrisel/fix-coordinate-descent-memory-l…
…ayout

[MRG+1] FIX consistency of memory layout for linear CD solver

@ogrisel ogrisel merged commit 1025982 into scikit-learn:master Oct 5, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -205,7 +205,8 @@ def graph_lasso(emp_cov, alpha, cov_init=None, mode='cd', tol=1e-4,
d_gap = np.inf
for i in range(max_iter):
for idx in range(n_features):
sub_covariance = covariance_[indices != idx].T[indices != idx]
sub_covariance = np.ascontiguousarray(
covariance_[indices != idx].T[indices != idx])

This comment has been minimized.

@amueller

amueller Oct 12, 2015

Member

Shouldn't this be

mask = indices != idx
covariance_[np.ix_(mask, mask)]

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux via email Oct 13, 2015

Member

This comment has been minimized.

@amueller

amueller Oct 13, 2015

Member

@MechCoder do you want to see if that is better and do a quick PR?

@@ -241,8 +241,7 @@ def sparse_encode(X, dictionary, gram=None, cov=None, algorithm='lasso_lars',
n_components = dictionary.shape[0]
if gram is None and algorithm != 'threshold':
# Transposing product to ensure Fortran ordering

This comment has been minimized.

@amueller
double tol, object rng,
double l2_reg,
np.ndarray[double, ndim=2, mode='fortran'] X,
np.ndarray[double, ndim=2] Y,

This comment has been minimized.

@amueller

amueller Oct 12, 2015

Member

so Y doesn't get a mode?

np.ndarray[double, ndim=2] Y, int max_iter,
double tol, object rng,
double l2_reg,
np.ndarray[double, ndim=2, mode='fortran'] X,

This comment has been minimized.

@amueller

amueller Oct 12, 2015

Member

This seems a lot saver ^^

@amueller

This comment has been minimized.

Member

amueller commented Oct 12, 2015

a deterministic regression test would be nice.
Thanks so much for fixing this!

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