Repository navigation
Fix analytic Jacobian for tied parameters in non-linear modeling fitters - #20711
Open
tepickering wants to merge 4 commits into
Open
tepickering wants to merge 4 commits into
tepickering wants to merge 4 commits into
Conversation
When a model has tied parameters, _NonLinearLSQFitter._wrap_deriv dropped the derivative columns of the tied parameters, losing their contribution to the derivatives with respect to the free parameters they depend on. As a result LevMarLSQFitter, TRFLSQFitter, DogBoxLSQFitter and LMLSQFitter could stop away from the least-squares minimum when using the analytic Jacobian. The full derivative is now multiplied by the Jacobian of the mapping from free to full parameters, with the rows for tied parameters estimated by central differences of the tied callables. Fixes astropy#20664
Contributor
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
Contributor
Author
|
Looks like CI is happy. There are only two commits and one is just a changelog entry rename so feel free to squash-merge. |
pllim
reviewed
Oct 25, 2026
Member
|
@scttfrdmn are you satisfied with this proposed solution? Thanks, all. |
This branch has not been deployed
This file contains hidden or 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
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.
Description
This pull request is to address the analytic Jacobian being wrong for models with tied parameters in the non-linear fitters (
LevMarLSQFitter,TRFLSQFitter,DogBoxLSQFitter,LMLSQFitter)._NonLinearLSQFitter._wrap_derivcomputed the full analytic derivative and then deleted the columns of both the fixed and the tied parameters. That is correct for fixed parameters, but for a tied parameterp_t = f(p_free)it drops the∂y/∂p_t · ∂f/∂p_freeterm from the derivative with respect top_free, so fits could stop away from the least-squares minimum (see #20664 for the impact on [O III] doublet fits inspecutils).The full derivative is now multiplied by the Jacobian of the mapping from free to full model parameters: identity rows for free parameters, zero rows for fixed ones, and rows for tied parameters estimated by central differences of the
tiedcallables (which can be arbitrary functions of the model, so their derivatives are not known in advance). Models with only fixed parameters take the same path as before. The cost is two evaluations of the tie expressions per free parameter per Jacobian call.This is the same factorization used by the emission-line fitting in FastSpecFit (
J = W · M · J_I · J_S, seepy/fastspecfit/emline_fit/sparse_rep.py), whereJ_Sis known exactly because ties there are restricted to fixed multiplicative factors. No code from it is used here.Tests added:
test_tied_par_analytic_deriv: the [O III] 4959/5007 doublet case from the issue, comparing each fitter (with the analytic Jacobian) againstscipy.optimize.leastsqon the equivalent function of the free parameters only. Fails onmain.test_wrap_deriv_tied_chain_rule: compares the Jacobian returned by_wrap_derivagainst finite differences for a model with several ties and a fixed parameter, for bothcol_fit_deriv=TrueandFalse, and checks the model parameters are left unchanged. Fails onmain.While working on this I found that tied expressions depending on other tied parameters see values from the previous call; that is reported separately in #20710 and is not changed here.
Fixes #20664
AI Disclosure
This pull request was developed with Claude Code using Claude Opus 5.5 (
claude-opus-5-5). It was used to investigate the issue (including comparing with the approach in FastSpecFit), reproduce the bug, and draft the code change, tests, changelog entry, and this description. I reviewed the changes and verified the tests fail onmainand pass with the fix, and that the fullastropy.modelingtest suite passes.Merge method