hotfix for segfault on mkl #12637
hotfix for segfault on mkl #12637
Conversation
|
This basically bails as soon as things go south, trying to keep solving afterwards likely means wasted work anyway. Maybe @agramfort @GaelVaroquaux can confirm? |
|
This will get rid of the segfault but will raise an exception on CircleCI instead right? I pushed an empty commit with |
|
No, it will not. This whole think is inside a big try/except. Not is just raises it a bit earlier than it was before. |
|
This implementation basically uses exceptions to monitor convergence / try out parameters. |
|
Ah OK I missed that in the diff, that makes sense. |
|
Does it? I'm not sure ;) |
There was a problem hiding this comment.
LGTM. Ideally it would be great to have test to trigger this case in the test suite but no need to delay the release for this.
|
circle passed @lesteve ;) |
sklearn/covariance/graph_lasso_.py
Outdated
| alpha_min=alpha / (n_features - 1), copy_Gram=True, | ||
| eps=eps, method='lars', return_path=False) | ||
| # Update the precision matrix | ||
| if not np.isfinite(coefs).all(): |
There was a problem hiding this comment.
Definitely not an expert on this. Is there a feeling on how likely/unlikely this check (inside a double loop) could affect performance in the well-conditioned case?
There was a problem hiding this comment.
That's a valid concern :-/
There was a problem hiding this comment.
I'm pretty sure it would be fine to pull it out one level. I tried to fail early but maybe checking less often is more important.
|
looks like it's still good. |
|
Great!
One trick for faster checks for nans is to do np.isnan(np.sum(X)). I seem
to remember that it is faster. Checking for nans is slow, if I remember
correctly.
|
|
should we do that? In the current place in the loop I'm not very concerned. |
|
should we do that? In the current place in the loop I'm not very concerned.
Not sure. I was just sharing my experience.
|
There was a problem hiding this comment.
If no one is opposed, should we merge this?
|
Good work, @amueller! Is there any harm to changing it to |
|
looks to be about twice as fast on my machine to do sum. I can do that. |
|
I'll merge once it's green, then rebase again and tag |
|
@jnothman how can I add on to your rebase? I'm a bit confused what you ran. |
|
didn't run docbuild again but it's unlikely this will have an impact. |
* probably fixing the segfault * Trigger CircleCI [doc build] * move isfinite out one level[doc build] * slightly faster finiteness check
|
Just cherry-pick. I can tell you what my rebase commands are later when I'm
sitting down.
|
|
that's what I ended up doing. I tried to use the commands you posted earlier but it didn't look right. |
|
I'd usually do something equivalent to git checkout -b tmp
git reset --hard upstream/master
git rebase -i for0.20.1
# resolve any issues in rebase
git checkout for0.20.1
git reset --hard tmp
git push |
* probably fixing the segfault * Trigger CircleCI [doc build] * move isfinite out one level[doc build] * slightly faster finiteness check
This reverts commit 5946e3c.
This reverts commit 5946e3c.
* probably fixing the segfault * Trigger CircleCI [doc build] * move isfinite out one level[doc build] * slightly faster finiteness check
I think this will fix the mysterious segfault in #12630