Fix Segmentation Fault and ZeroDivisionError in Group Lasso
#292
Conversation
| @@ -418,8 +418,11 @@ cpdef celer_grp( | |||
| &inc) / lc_groups[g] | |||
| norm_wg += w[j] ** 2 | |||
| norm_wg = sqrt(norm_wg) | |||
| bst_scal = max(0., | |||
| if norm_wg != 0.: | |||
| bst_scal = max(0., | |||
There was a problem hiding this comment.
I'm perhaps lacking context about the code, namely I ignore the purpose of bst_scal.
I just followed commun sense to handle the case of norm w being zero.
There was a problem hiding this comment.
bst_scal is for BockSofthThresholding scaling : the formula for the BST of wg at level lambda is:
wg * max(0, 1 - lambda / norm(wg)) aka 0 if norm(wg) < lambda, and (1 - lambda/norm(wg)) wg otherwise
I'm curious, how do you end up with a vanishing wg after a gradient step ? This should not happen with probability 1, I'm guessing a 0 group X_g ?
celer/group_fast.pyx
Outdated
| @@ -89,7 +89,7 @@ cpdef floating dnorm_grp( | |||
|
|
|||
| else: # scaling only with features in C | |||
| for g_idx in range(ws_size): | |||
| if weights[g] == INFINITY: | |||
| if weights[g_idx] == INFINITY: | |||
| continue | |||
|
|
|||
| g = C[g_idx] | |||
There was a problem hiding this comment.
are you sure that it's not g (defined L95, so def should be moved above) which should be used here? weights has size n_groups, not ws_size
There was a problem hiding this comment.
Yes you are right,
We can insert line 95 right after Line 91 and keep the rest as it is
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
=======================================
Coverage 88.53% 88.53%
=======================================
Files 15 15
Lines 1143 1143
Branches 127 127
=======================================
Hits 1012 1012
Misses 101 101
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
|
@mathurinm, don't mind the 10 files diff, it is just GitHub glitch |
|
@Badr-MOUFAD my solution in that case is to merge main into the branch, now it is reduced to 3 files |
|
Thanks @Badr-MOUFAD |
Context
I run unittest of
skglmand got an error for celer namely segmentation fault.The tests were run on a Mac. Surprisingly, the problem doesn't arise for Linux (no errors in skglm CI)
Contributions of the PR
After debugging, the segmentation fault (out of bound index) comes from an un initialized variable.
Other errors, due to changes of scikit-learn API, were fixed as well.