Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 468 (Levenberg-Marquardt damping term) #473

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

Conversation

dwpaley
Copy link
Contributor

@dwpaley dwpaley commented May 8, 2020

The damping term doesn't blow up so rapidly when the refinement is fully
converged (i.e. objective stops changing)

#468

@luc-j-bourhis @nksauter Any objection?

@nksauter
Copy link
Contributor

nksauter commented May 8, 2020

No objections that I know of at this time.

@dwpaley
Copy link
Contributor Author

dwpaley commented May 9, 2020

I see the tst_commandline_anomrefine failure on Centos 8/python36, but I couldn't reproduce it in 300 trials in a VM. I know that was one of the builds affected by the PR from last night, so I'll rebase and see if the tests pass.

The damping term doesn't blow up so rapidly when the refinement is fully
converged (i.e. objective stops changing)
@dwpaley dwpaley force-pushed the LM_step_threshold branch from 53f5d2c to 30af3d0 Compare May 9, 2020 14:44
@dwpaley dwpaley marked this pull request as ready for review May 10, 2020 21:17
@luc-j-bourhis
Copy link
Contributor

Interesting. I would say that if the damping terms blows up when the refinement is fully converged, that's because the termination criteria on gradient and step are too optimistic (methods had_too_small_a_step and has_gradient_converged_to_zero governed by step_threshold and gradient_threshold respectively). Could you try reducing those two values and see whether it solves your problem? The theory behind the test you want to change is that rho shall not be too small. So actually, some implementations impose rho > eps for some small positive eps instead.

Then, @pcxod L-M is used in Olex 2, isn't it? Any opinion on that?

@dwpaley
Copy link
Contributor Author

dwpaley commented May 12, 2020

Actually this only happens when step_threshold and gradient_threshold are set to 0. In that case it runs iterations until actual_decrease is identically 0; then it starts doing

      else:
        self.non_linear_ls.step_backward()
        self.mu *= nu
        nu *= 2

until (in ~30 more iterations) mu is in the 1e150 range and the damped step norm becomes identically 0 and the refinement stops in the middle of the iteration. Then the problem is only detectable because the esds are calculated based on the (extremely over-) damped normal matrix.

If you want to leave the rho test as it is, I suggested a different solution in the issue I wrote, where the iteration always goes to the end, finishing with build_up. Then esds will be calculated from the un-damped normal equations as Sheldrick recommends: http://shelx.uni-goettingen.de/shelxl_html.php#DAMP The only problem is, if the refinement was unstable (hence the need for L-M refinement) then it might crash with a Cholesky failure when attempting the final un-damped solution.

Otherwise some other more reliable way to detect convergence. I have more thoughts on this but it might exceed the scope of this PR.

Dan

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

Successfully merging this pull request may close these issues.

3 participants