-
Notifications
You must be signed in to change notification settings - Fork 176
Fix Hessian calculation for NonlinearVariationalSolver block
#4641
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
Conversation
angus-g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to cache the intermediate adjoint solution on the block rather than relying on the solver output. I've tested that this does indeed reproduce and subsequently fix the issue.
Also sweeps up a bunch of warnings in the Hessian tests which is nice.
|
@angus-g I fixed one small annotation error (the only test fail on the last commit) and a few more warnings. |
|
@dham pointed out that my original fix was preventing pyadjoint from cleaning up the adjoint state properly with this call: I've redone the fix to use the existing |
This PR fixes a bug in the Hessian calculation when calling
solver.solvemultiple times on the sameNonlinearVariationalSolver.The Hessian calculation requires the adjoint variable. The
GenericSolveBlockstores this on the block, so if there are multiple calls tofiredrake.solvethen each block has it's own adjoint solution and everything works fine.However, the
NonlinearVariationalSolveBlockstored the solution of the cached adjoint solver. This solver is shared between all blocks created by the sameNonlinearVariationalSolverso if there are multiple blocks then they overwrite each others adjoint solutions.This PR fixes this by making each block stash their own adjoint solution to reuse when calculating the Hessian action.
This is a quickfix to solve the issue on release. There's a bigger refactor of the cached solvers to improve the performance in this PR: #4638 but that should probably go into main.