mathurinm / celer

Fast solver for L1-type problems: Lasso, sparse Logisitic regression, Group Lasso, weighted Lasso, Multitask Lasso, etc.
https://mathurinm.github.io/celer/
BSD 3-Clause "New" or "Revised" License
204 stars 33 forks source link

Fix Segmentation Fault and ``ZeroDivisionError`` in Group Lasso #292

Closed Badr-MOUFAD closed 7 months ago

Badr-MOUFAD commented 8 months ago

Context

I run unittest of skglm and 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.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.53%. Comparing base (c1b564c) to head (138bd97).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/mathurinm/celer/pull/292/graphs/tree.svg?width=650&height=150&src=pr&token=hCZ8E3PSmY&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://app.codecov.io/gh/mathurinm/celer/pull/292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/mathurinm/celer/pull/292/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/mathurinm/celer/pull/292/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/mathurinm/celer/pull/292?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [celer/dropin\_sklearn.py](https://app.codecov.io/gh/mathurinm/celer/pull/292?src=pr&el=tree&filepath=celer%2Fdropin_sklearn.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y2VsZXIvZHJvcGluX3NrbGVhcm4ucHk=) | `95.95% <ø> (ø)` | | | [celer/tests/test\_mtl.py](https://app.codecov.io/gh/mathurinm/celer/pull/292?src=pr&el=tree&filepath=celer%2Ftests%2Ftest_mtl.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y2VsZXIvdGVzdHMvdGVzdF9tdGwucHk=) | `100.00% <ø> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/mathurinm/celer/pull/292?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/mathurinm/celer/pull/292?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [c1b564c...138bd97](https://app.codecov.io/gh/mathurinm/celer/pull/292?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
Badr-MOUFAD commented 8 months ago

@mathurinm, don't mind the 10 files diff, it is just GitHub glitch

mathurinm commented 8 months ago

@Badr-MOUFAD my solution in that case is to merge main into the branch, now it is reduced to 3 files

mathurinm commented 7 months ago

Thanks @Badr-MOUFAD