Skip to content

Fix analytic Jacobian for tied parameters in non-linear modeling fitters - #20711

Open
tepickering wants to merge 4 commits into
astropy:mainfrom
tepickering:fix-tied-jacobian
Open

tepickering wants to merge 4 commits into
astropy:mainfrom
tepickering:fix-tied-jacobian

Conversation

@tepickering

@tepickering tepickering commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

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_deriv computed 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 parameter p_t = f(p_free) it drops the ∂y/∂p_t · ∂f/∂p_free term from the derivative with respect to p_free, so fits could stop away from the least-squares minimum (see #20664 for the impact on [O III] doublet fits in specutils).

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 tied callables (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, see py/fastspecfit/emline_fit/sparse_rep.py), where J_S is 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) against scipy.optimize.leastsq on the equivalent function of the free parameters only. Fails on main.
  • test_wrap_deriv_tied_chain_rule: compares the Jacobian returned by _wrap_deriv against finite differences for a model with several ties and a fixed parameter, for both col_fit_deriv=True and False, and checks the model parameters are left unchanged. Fails on main.

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 on main and pass with the fix, and that the full astropy.modeling test suite passes.

  • I certify that I am human and that I take full responsibility for this pull request including all interactions with reviewers.

Merge method

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

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
@github-actions

Copy link
Copy Markdown
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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@tepickering

tepickering commented Oct 24, 2026 •

Copy link
Copy Markdown
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.

@tepickering
tepickering marked this pull request as ready for review October 24, 2026 20:58
@tepickering
tepickering requested a review from a team as a code owner October 24, 2026 20:58
@pllim pllim added the Bug label Oct 25, 2026
@pllim pllim added this to the v8.1.0 milestone Oct 25, 2026
Comment thread docs/changes/modeling/20711.bugfix.rst Resolved
@pllim
pllim requested review from WilliamJamieson and nden October 25, 2026 21:44
@pllim

pllim commented Oct 25, 2026

Copy link
Copy Markdown
Member

@scttfrdmn are you satisfied with this proposed solution? Thanks, all.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants