Closed pavanramkumar closed 4 years ago
closes #319
@pavanramkumar our current tests don't test if we stop our iterations too late. Perhaps we should have a test where we know that it converges in less than x iterations (e.g., with scikit-learn data)?
assert glm.n_iter_ < 15
or something like that?
@pavanramkumar our current tests don't test if we stop our iterations too late. Perhaps we should have a test where we know that it converges in less than x iterations (e.g., with scikit-learn data)?
assert glm.n_iter_ < 15
or something like that?
The problem with this is that we cannot easily distinguish good and bad failures. Some optimization loops with warm restarts legit converge in 10 iterations. So I'd say let's wait to improve this
@jasmainak tests pass now. good to merge?
Merging #327 into master will decrease coverage by
0.03%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
- Coverage 75.48% 75.44% -0.04%
==========================================
Files 4 4
Lines 673 672 -1
Branches 148 147 -1
==========================================
- Hits 508 507 -1
Misses 128 128
Partials 37 37
Impacted Files | Coverage Δ | |
---|---|---|
pyglmnet/pyglmnet.py | 81.04% <100%> (-0.04%) |
:arrow_down: |
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 7f5dd22...3966252. Read the comment docs.
@pavanramkumar I'm merging this so I can use it for some testing in my PR. I'll update the what's new in my PR
A few things to note (see review comments):
The criterion based on (relative) parameter change per iteration is sufficient for all tests to pass and speed up all examples despite strict default tolerance (
tol=1e-6
).However, I am leaving the gradient norm based criterion in the code for readability sake. My main concern is that to make the gradient-based convergence work well, we have to calculate the projected gradient (i.e. what remains after the proximal operator has been applied) and then take the norm on it. Unfortunately that required more digging into the implementation of
_prox
for all corner cases and I ran out of time. So I would recommend removing this criterion entirely for this PR.As a consequence of this PR, one test fails:
test_random_state_consistency
. However the failure is easily understood and can be traced back to cold (random) vs. warm initialization of beta. I've recommended a modification to the test: useassert_allclose
instead ofassert_equal
(commented out for now; I wanted to demo the failed test first)