Skip to content

Fix residuals space of modified King fits and add reinit!() checks#116

Merged
ChrisRackauckas merged 6 commits into
masterfrom
king
Jul 3, 2026
Merged

Fix residuals space of modified King fits and add reinit!() checks#116
ChrisRackauckas merged 6 commits into
masterfrom
king

Conversation

@JamesWrigley

Copy link
Copy Markdown
Member

Some more things found by auditing with Claude:

  • d683795: modified King fits are solved using a slightly different function (I assume for numerical stability) and the residuals were calculated in that function space. Now we return the residuals in velocity space, which is consistent with the rest of the codebase such that res == y - ŷ holds.
  • c8fd4e7: removes some unused code and cleans up some tests.
  • e14ecfd: adds checks to reinit!() for nonlinear fits so that it will explicitly error out if the input lengths are wrong or unsupported by the cache.

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

The residuals are computed in E² but that's not very intuitive and is not
consistent with the rest of the API.
- These fit types don't properly support `reinit!()` so they always build a cache
  inside of `solve!()`, meaning that their caches `nonlinear_cache` fields are
  never used.
- Delete the custom Jacobian method for King fits. This is unused now that we
  use the delta method for constructing the covariance matrix.
- Fix the modified King fit tests to pass x/y in the right order, previously
  they were swapped.
@JamesWrigley JamesWrigley self-assigned this Jul 1, 2026
@JamesWrigley

Copy link
Copy Markdown
Member Author

Sneaked in one more fix in 49bbb66, which fixes #92.

This would previously introduce small errors by first scaling and then inverting
the scaling. Also made the residuals return a vector instead of a matrix.
@JamesWrigley

JamesWrigley commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Two more fixes found by Fable:

  • 14b6ac6: expsum fits were modifying the inputs during the solve 🫠 It did invert the transform but it still causes floating point error. Now we copy and modify the inputs instead.
  • 7976513: when passing y=nothing to nonlinear fits, the objective function is treated as returning the residual directly. But in such cases sigma would be silently dropped from but kept in the problem, which meant that the statistics methods would wrongly assume that sigma was used and calculate the wrong results. Now we always use sigma. Also made some other minor fixes for in-place/y=nothing solutions.

CurveFit supports passing `y=nothing` when creating a NonlinearCurveFitProblem,
which means that we treat the function as returning residuals directly. But
__wrap_nonlinear_function() would silently drop sigma when y is nothing while
leaving it in the original problem, which meant that the statistics methods
would assume sigma was used during the fit and apply it incorrectly. Now we
always use sigma if it's passed.

Also fixed support for `sol(x)` for in-place functions, and made `nobs()` able
to handle solutions where y is nothing.
@ChrisRackauckas ChrisRackauckas merged commit 11a6634 into master Jul 3, 2026
19 checks passed
@ChrisRackauckas ChrisRackauckas deleted the king branch July 3, 2026 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants