Project

General

Profile

Bug #1593

step-size scaling in lbfgs doesn't work as intended

Added by Mark Abraham about 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Low
Assignee:
-
Category:
mdrun
Target version:
-
Affected version - extra info:
likely all versions
Affected version:
Difficulty:
uncategorized
Close

Description

When converting minimize.c to C++, clang-static-analyzer notices that stepsize gets scaled by golden ratio numbers around line 1960, but then stepsize is unilaterally set to the value of c around line 2142. Removing the latter suppresses the error from the static analyzer, and seems consistent with trying to do scaling of step size. See clang-static-analysis errors at https://gerrit.gromacs.org/#/c/4010/. Thoughts?


Related issues

Related to GROMACS - Bug #2641: Possible l-bfgs improvementsClosed

Associated revisions

Revision 4a6c1455 (diff)
Added by Mark Abraham almost 5 years ago

Convert remaining callers of do_force to C++

Major callers are converted in other patches.

Removed unused variables, used std::min/max, named constants, put
maximum length on sscanf floats.

Introduced an instance of bMaster. Initialized gstat to and later
checked for NULL before use. These help static analysis reason
correctly. Previous code functioned correctly in practice, but correct
analysis would require cross-function analysis that cr, when passed as
non-const, is not actually modified. Fixed a warning about stepsize
not being used in L-BFGS, and added more comments to this algorithm
while editing the code to help us understand what it does.
The predict_shells() function has been modified to use the
gmx_mtop_atomnr_to_atom() call for case 2; the previous version would
use an uninitialized pointer.

Fixes #1593.

Change-Id: I54f238cedc78aadb0dad080ea3b28f001dce8d94

History

#1 Updated by Mark Abraham about 5 years ago

  • Description updated (diff)

#2 Updated by Erik Lindahl about 5 years ago

  • Status changed from New to Fix uploaded

It was slightly more complicated than that. The stepsize variable was used to indicate the step taken (which is used in the correction factor application a few lines further down), but I've introduced a new var for this instead.

Tested to work on 2-3 different systems - it produces the same energy minima as before.

#3 Updated by Mark Abraham about 5 years ago

But the static analyzer points out that stepsize is now unused, rather merely over-written... http://jenkins.gromacs.org/job/clang-static-analyzer_Gerrit_master/2377/javadoc/... and there is some new unrelated issue in shellfc, also.

#4 Updated by Rossen Apostolov over 4 years ago

  • Status changed from Fix uploaded to Closed

closing since the patch has been merged last year

#5 Updated by Mark Abraham 8 months ago

  • Related to Bug #2641: Possible l-bfgs improvements added

Also available in: Atom PDF