Closed pavanramkumar closed 4 years ago
I think the intuition of @cxrodgers is right that close to the true solution the Hessian term is small and therefore the update term explodes. I've put in a PR #348 that only uses the Hessian reciprocal if it is sufficiently large and added a test to ensure the estimated coefficients are not np.nan
@cxrodgers if you have time, it will be great to test on your dataset to check if it alleviates your convergence problem before we merge this
Merging #348 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #348 +/- ##
=======================================
Coverage 75.66% 75.66%
=======================================
Files 4 4
Lines 678 678
Branches 149 149
=======================================
Hits 513 513
Misses 128 128
Partials 37 37
Impacted Files | Coverage Δ | |
---|---|---|
pyglmnet/pyglmnet.py | 81.25% <100%> (ø) |
:arrow_up: |
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 e694585...f05b744. Read the comment docs.
@geektoni this PR may also address the nan issue for cdfast in #274
@jasmainak @cxrodgers
i have also now tested with the dataset provided in #289
i verified that both cdfast
and batch-gradient
converge with smoothly decreasing loss trace across poisson
, softplus
and gaussian
distrs.
@pavanramkumar can you update whats_new.rst
?
@jasmainak updated whats_new.rst
and tutorial.rst
Merged, thanks @pavanramkumar !
closes #289