Skip to content

Correct BFGS update#26

Closed
timothy-nunn wants to merge 1 commit intomainfrom
update-bfgs
Closed

Correct BFGS update#26
timothy-nunn wants to merge 1 commit intomainfrom
update-bfgs

Conversation

@timothy-nunn
Copy link
Copy Markdown
Collaborator

I believe that the BFGS update of the Lagrangian function Hessian in PyVMCON is incorrect. If the revision of the B matrix were incorrect, this could explain the "Workspace allocation error" being observed in ukaea/PROCESS#3619 (which was happening because B was not positive definite and hence the quadratic programming problem failed because it was non-convex, the error would now have a different message because of #21).

Crane report

The Crane report states the update of the B matrix occurs as follows:
image

It should be noted there is a typo in the second term that was identified during PyVMCON's development and it should read $\frac{\gamma\gamma^T}{\xi^T\gamma}$. (In some other places the denominator is $\gamma^T\xi$ but that does not change the result.)

Powell

Powell's paper agrees with the Crane report (as the Crane report is a summary of Powell's paper) except for the second term is correct.

image

BFGS Update

The BFGS wiki states that the update occurs as follows:
image

Note that this disagrees with both Powell's paper and the Crane report in that the second terms denominator (the first term in the other two papers) reads $B\xi\xi^TB^T$: the final B is transposed.

When I update PyVMCON to reflect this, none of the unit tests fail but it does allow the skipped PROCESS file to converge (albeit after 177 iterations).

@timothy-nunn timothy-nunn requested review from jonmaddock and mkovari and removed request for jonmaddock April 11, 2025 09:56
@timothy-nunn timothy-nunn marked this pull request as draft April 11, 2025 11:05
@timothy-nunn
Copy link
Copy Markdown
Collaborator Author

As @mkovari has pointed out, the B matrix should be symmetric so a transpose should not change anything?

Having looked through the mathematics of the BFGS update equation, its possible PyVMCON implements it incorrectly and is not calculating the update matrices correctly. We should verify the shapes of the intermediate update matrices to ensure parity with NEA implementation VMCON.

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.

1 participant