QB3 / sparse-ho

Fast hyperparameter settings for non-smooth estimators:
http://qb3.github.io/sparse-ho
BSD 3-Clause "New" or "Revised" License
38 stars 15 forks source link

[FIX] Fix warm start and stopping criterion for Jacobian computation #128

Closed Klopfe closed 2 years ago

Klopfe commented 2 years ago

This is the reason why tests are slow.

codecov-commenter commented 2 years ago

Codecov Report

Merging #128 (2033d8e) into master (c909d8c) will increase coverage by 0.06%. The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   79.43%   79.49%   +0.06%     
==========================================
  Files          41       41              
  Lines        3029     3048      +19     
  Branches      318      322       +4     
==========================================
+ Hits         2406     2423      +17     
- Misses        533      534       +1     
- Partials       90       91       +1     
Flag Coverage Δ
unittests ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sparse_ho/tests/test_docstring_parameters.py 75.53% <ø> (ø)
sparse_ho/tests/test_models.py 82.20% <84.21%> (+1.01%) :arrow_up:
sparse_ho/algo/implicit_forward.py 89.79% <100.00%> (+0.43%) :arrow_up:
sparse_ho/models/enet.py 89.09% <100.00%> (ø)
sparse_ho/models/lasso.py 85.29% <100.00%> (ø)
sparse_ho/models/logreg.py 87.43% <100.00%> (ø)
sparse_ho/models/svm.py 71.36% <100.00%> (ø)
sparse_ho/models/svr.py 80.63% <100.00%> (ø)
sparse_ho/models/wlasso.py 86.03% <100.00%> (ø)

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 c909d8c...2033d8e. Read the comment docs.

Klopfe commented 2 years ago

So I added a condition check to be sure that the jacobian is not exactly the same. When it was the case: this stopping criterion condition t: np.abs(objs[-2] - objs[-1]) < np.abs(objs[-1]) * tol_jac

when objs[-1] is 0 it was never met.

Klopfe commented 2 years ago

Tests run in 30s now

mathurinm commented 2 years ago

Cool ! Since we're on it, a few questions : how can we get 0 as obj ? why is it related to warmstart/jacobian not changing ? And why do we need abs around obj ? In that case it may be safer to compare objs[-1] to 1e-12 or something ?

Klopfe commented 2 years ago

we needed a stopping criterion when we compute the Jacobian alone (implicit forward). Technically, you can see that as solving a linear problem using coordinate descent (in some cases, for example the LASSO). So what we did Quentin and I, is that we took the quadratic form, meaning that if we solve Ax = b we look at the value of norm(x^T A x - x^T b) and use that for our "naive" stopping criterion.

Now how that can be exactly 0 is not very clear for me as I would expect that it would be up to a tolerance. So I am still thinking about that.

mathurinm commented 2 years ago

Looks very good like this, one last thing: can you add a test, that would fail on master, and succeed here ? I'm thinking of doing a warmstart and checking that we exit after 1 iterations (thorugh len(res_norms) instead of going up to max_iter