search

mathurinm / andersoncd

This code is no longer maintained. The codebase has been moved to https://github.com/scikit-learn-contrib/skglm. This repository only serves to reproduce the results of the AISTATS 2021 paper "Anderson acceleration of coordinate descent" by Quentin Bertrand and Mathurin Massias.
BSD 3-Clause "New" or "Revised" License
18 stars 6 forks source link

Adding MCP class in penalties #36

Closed SalimBenchelabi closed 3 years ago

SalimBenchelabi commented 3 years ago

closes #35

codecov-commenter commented 3 years ago

Codecov Report

Merging #36 (8062c75) into master (05df374) will decrease coverage by 3.63%. The diff coverage is 64.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   60.93%   57.30%   -3.64%     
==========================================
  Files          11       15       +4     
  Lines         919     1260     +341     
  Branches      198      254      +56     
==========================================
+ Hits          560      722     +162     
- Misses        302      470     +168     
- Partials       57       68      +11
Impacted Files Coverage Δ
andersoncd/tests/test_docstring_parameters.py 74.63% <ø> (+10.14%) :arrow_up:
andersoncd/logreg.py 37.97% <28.12%> (-18.71%) :arrow_down:
andersoncd/lasso.py 37.76% <37.50%> (-21.09%) :arrow_down:
andersoncd/group.py 62.40% <39.28%> (-11.79%) :arrow_down:
andersoncd/penalties.py 40.17% <40.17%> (ø)
andersoncd/utils.py 32.87% <52.38%> (+12.66%) :arrow_up:
andersoncd/solver.py 79.50% <79.50%> (ø)
andersoncd/plot_utils.py 64.44% <87.50%> (+26.94%) :arrow_up:
andersoncd/__init__.py 100.00% <100.00%> (ø)
andersoncd/estimators.py 100.00% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dc14d9d...8062c75. Read the comment docs.

mathurinm commented 3 years ago

@QB3 I don't know if we should add one estimator per Penalty. For very popular ones (Lasso, ElasticNet) existing in sklearn, yes, as it provides a dropin replacement . But it's a pain to maintain the docstring harmonized (they share 90 % of their code) ; when we add group penalties and other datafits, we will easily have 2 datafits 2 group/nongroup 5 penalties

It feels like we should do Estimator(datafit, penalty), no ? But I don't know how we will comply with sklearn API (eg GridSearchCV) as sklearn estimators should be instanciated without parameters

QB3 commented 3 years ago

we will easily have 2 datafits 2 group/nongroup 5 penalties. It feels like we should do Estimator(datafit, penalty), no ?

I agree that we will not instantiate all the combinations in estimators like Lasso or Enet, since the MCP with a quadratic datafit is somewhat well know in the stat community, I would not be shocked to have an estimator for it.

It feels like we should do Estimator(datafit, penalty), no ? But I don't know how we will comply with sklearn API (eg GridSearchCV) as sklearn estimators should be instantiated without parameters

+1 this is the ultimate goal, but it may be a lot of work to make it work with gridsearchCV

mathurinm commented 3 years ago

Thanks @SalimBenchelabi @QB3 ! We will add an example in a subsequent PR