feat: optimize stretch using a Hessian matrix#204
Conversation
| gra = gra.flatten() | ||
| return fun, gra | ||
|
|
||
| def hessian(stretch_vec): |
There was a problem hiding this comment.
do we need a test for this?
There was a problem hiding this comment.
Good idea. I have added a test checking for some of the expected properties (right shape, symmetric, correct ordering/cross-coupling).
| return fun, gra | ||
|
|
||
| def _regularize_function_hessian(self, stretch): | ||
| residuals, d_stretch_comps, dd_stretch_comps = ( |
There was a problem hiding this comment.
this could use some dicumentation and also maybe a more descriptive variable name. Stretch is a verb usually and I would expect a noun here.
We were pretty lazy about documenting code in the early part of the project because we wanted it to get going from a low place, but we are at the point of the project where we can take the time to make the code more maintainable moving forward.
There was a problem hiding this comment.
Sure, I have added a numpy style docstring as used for some of the other functions.
As for renaming stretch, I am open to it, but that would be a broader refactor that I'd be more comfortable carrying out once the current PRs are merged. The word "stretch" is used for this variable name to be consistent with the class variable stretch, which is broadly used throughout the entire code. It, along with components and weights are some of the most-used words in the entire file. Replacing it with a longer term would also have the side effect of visually bloating certain lines of code that use the variable repeatedly in calculations.
That being said, we could try something like stretch_factors which makes it more clear we are dealing with a noun. I'd just prefer to hold off on it for now.
There was a problem hiding this comment.
Ok. The code is really not very readable ATM which would make it harder to maintain in the future, but I am ok to put this off for now.
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
Please see my comment. I will merge like this but we really need to refactor to make the code much more readable now it is working
There was a problem hiding this comment.
Always start descriptions of parameters with "The"
2 participants
This is another PR bringing us closer to parity with MATLAB. Proper use of a Hessian was key to the speed and quality of convergence they were able to obtain. I had tried this a few times before, but it never worked even though the Hessian had the right shape. By properly regularizing, it now works.