Slimmed-down Levenberg-Marquardt for nonlinear least squares #500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…copy-paste of custom_leastsq to simplish_lstsq)
…ty" in the CustomLSOptimizer behavior. Remove damping_clip as well, since that was only used for damping_mode != "identity".
…tomLMOptimizer when damping_basis=="diagonal_basis". I believe that since I already restricted damping_mode=identity in the previous commit that damping_basis has no special meaning anyway.
…ored out into their own functions
…s in simplerlm.py instead of customlm.py, so we can mark the whole customlm.py file as deprecated.
…d custom_leastsq to simplish_leastsq
rileyjmurray
changed the title
|
Ping @enielse for review, when you get a chance. Now is a good time for us to document why options are worth keeping in the SimplerLMOptimizer class and the simplish_leastsq function. I'd be all for removing even more options if you think some are unnecessary. |
…f boolean flag computed elsewhere) then (break or continue)" and moved them to where the value of the flag was originally determined.
… directly accesses elements of JTJ.
coreyostrove
approved these changes
Dec 10, 2024
|
Ping @sserita -- can you take a look at this? Once you approve we can merge this in. |
sserita
approved these changes
Dec 16, 2024
|
WOOO! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pyGSTi's nonlinear least squares solver is a custom implementation with Levenberg-Marquardt with a dizzying array of options. The core algorithm is over 800 lines long and has 10 levels of indentation at its deepest point. The complexity of this existing implementation makes it basically impossible to extend.
This PR introduces slimmed-down infrastructure for nonlinear least squares. I started by copying customlm.py into a new file called simplerlm.py, then I removed one feature at a time in a series of commits. "Removal" of a feature meant making the default behavior for a given option the only behavior.
Notes
I tried to use ChatGPT to help split the monolithic custom_leastsq function in customlm.py into simpler functions. This turned out to not work for reasons that I'll discuss off-GitHub if people are interested.
For the curious, I'm doing this because I want to extend our nonlinear least squares solver in two ways. First, I want it to support pytorch Tensors; right now we only support numpy arrays and whatever home-cooked thing we have for distributed-memory computations. Second, I want to experiment with optimization algorithms that rarely (if ever!) require evaluating the full Jacobian of circuit outcome probabilities.
I also edited a scaling test for MPI because it raised errors when I ran pytest.
Finally, I made slight extensions to the ArraysInterface classes. The new functions are there so that the simpler LM implementation never needs to directly index into JTJ.