Add proper accept/reject logic for LM optimizer #364
Conversation
facebook-github-bot
added
the
CLA Signed
luisenp
force-pushed
the
lep.better_lm_reject
branch
from
a0cd143
to
40c4e86
Compare
There was a problem hiding this comment.
Thanks for the comments! I didn't think of adding new tests, since the optimizer needs to go through all the new step logic. I guess it's possible to add some tests specifically for accept/reject, but that seems a bit cumbersome, since I think I'd need to mock a lot of things to be able to test it.
…at also return optional reject indices.
luisenp
force-pushed
the
lep.better_lm_reject
branch
from
990ac2c
to
16a0097
Compare
| den = (delta * (damping * delta + linearization.Atb.squeeze(2))).sum(dim=1) | ||
| rho = (last_err - new_err) / den | ||
| good_idx = rho > damping_accept | ||
| rho = (previous_err - err) / den |
There was a problem hiding this comment.
Wondering if we should rename previous_err and err to previous_cost and cost. In our convention, we assume err is vector.
| self._damping * up_damping_ratio, | ||
| self._damping / down_damping_ratio, |
There was a problem hiding this comment.
Maybe we should change the _damping update policy. In Ceres, up_damping_ratio/down_damping_ratio are also dynamically updated.
There was a problem hiding this comment.
In LM method, the self._damping might be unchanged if the the rho is neither good nor bad. Therefore, there should good_indices, bad_indices and accepted_indices, where self._damping is reduced, increased and unchanged, respectively.
* Refactored optimizer retract step so that retract and update happen separately. * Changed NonlinearOptimizer.step() so that it returns the error. * Renamed retract_optim_vars to a more descriptive name. * Changed Objective.update() so that it can also be given an ignore mask. * Replaced update_optimizer_state by a method called _complete_step, that also return optional reject indices. * Made _step private. Added some comments.
The previous version would adjust the damping depending on whether the step should be accepted or not, but it would nevertheless always take the step. This could result in divergence if the down damping ratio was too aggressive.
In the new version, if the step should be rejected, it's indeed rejected (and this is computed independently for each batch element).