Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

bug-fix: just-in-time update in SVRG; feature: averaging for SVRG #120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krishnap25
Copy link

@krishnap25 krishnap25 commented Mar 21, 2018

The just in time update for SVRG (formerly line 141 in svrg_fast.pyx) is incorrect when alpha > 0. In particular, the updates made in previous steps are scaled by a factor $(1 - \eta*\alpha)$ in each step, which has not been accounted for. My patch fixes this bug by maintaining the dense updates separately and adding a correction by summing the geometric series $\sum_{t'<t} (1-\eta*\alpha)^{t'}$ at inner iteration t.

Moreover, the patch also adds support for averaging inner iterates of SVRG, which has the same theoretical guarantees but exhibits better empirical performance. My implementation of averaging uses the sparse update scheme described by Bottou (2012).

I've tested the code by comparing against my trusted (dense) implementation of SVRG. The coefficients learnt my this patch match those of my implementation up to 1e-8.

@mblondel
Copy link
Member

Thanks for the patch! It would be nice to add a test case that fails without the patch. What we've done in other modules like SAGA is to add to the tests a simple pure Python implementation (without any optimization trick) and check that this implementation and the optimized Cython implementation indeed return the same result.

@krishnap25
Copy link
Author

The current update is mathematically incorrect. The bug is hard to detect because it does not affect the loss or predictions by a noticeable amount. However, the weights show a small drift from the unoptimized (dense) implementation and this drift grows over time when the same random examples are chosen. I'll add a test for this in a few days.

@mblondel
Copy link
Member

small drift from the unoptimized (dense) implementation

If you could add it to test_svrg.py and test the Cython implementation against it, that would be great.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants