Improve fitting performance of most non-linear fitters by Cadair · Pull Request #16673 · astropy/astropy

Cadair · 2024-07-04T13:31:06Z

This PR significantly speeds up fitting of models for most of the non-linear fitters. When profiling this, we found that a lot of the overhead was in the fact that the fitters evaluate the model using call, which includes several time-consuming pre- and post-evaluation steps. However, in the non-linear fitters, the models always have scalar parameters and we have control over the inputs to the models, so these checks are unnecessary. We can therefore call Model.evaluate directly which speeds things up a lot. For compound models, this can speed up the fitting by factors of 4x or more.

For a simple 1D Gaussian unconstrained fit:

Fit example

import time
import numpy as np
from astropy.modeling.models import Gaussian1D
from astropy.modeling.fitting import TRFLSQFitter

g = Gaussian1D()
fitter = TRFLSQFitter()

np.random.seed(12345)

x = np.linspace(-10, 10, 100)
y = np.exp(-(x - 1.1) ** 2 / 3) + np.random.uniform(-0.2, 0.2, 100)

start = time.time()
for iter in range(1000):
    g_fit = fitter(g, x, y)
end = time.time()

print(f"{end-start}")

This takes the run time for me from 3.02s on main to 2.19s on this PR.

With use_min_max_constraints and bounds set it goes from 3.64s to 2.2s.

If you change the model being fit to g = Gaussian1D() + Gaussian1D() then it takes 27.9s on main and 9.4s on this PR.

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.

github-actions · 2024-07-04T13:31:33Z

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.

github-actions · 2024-07-04T13:31:35Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

Cadair · 2024-07-04T15:59:47Z

The test fails are due to #16675

astrofrog · 2024-07-04T20:48:12Z

I've split out some of the changes here that relate to the Model API into #16677

docs/changes/modeling/16673.feature.rst

pllim · 2024-07-11T15:00:21Z

Please use the new perf change log category, see:

Added a 'perf' changelog entry type #16694

astrofrog · 2024-07-11T15:04:52Z

@pllim already done :)

pllim · 2024-07-11T15:09:00Z

I think you need to rebase because the bot does not know about "perf" here.

…de the objective function and avoiding modifying a model instance at every iteration.

Cadair · 2024-07-11T15:34:49Z

My kingdom for that update and rebase button on the github web ui

astrofrog · 2024-07-15T15:58:42Z

The asv benchmarks seem to be working! The one regression is a fluke.

astrofrog · 2024-07-15T16:12:04Z

For convenience:

2024-07-11T16:08:05.5964106Z | Change   | Before [ff59e9cf]    | After [2a05840e]    |   Ratio | Benchmark (Parameter)                                         |
2024-07-11T16:08:05.5966580Z |----------|----------------------|---------------------|---------|---------------------------------------------------------------|
2024-07-11T16:08:05.5968188Z | -        | 103±3ms              | 63.2±0.3ms          |    0.61 | modeling.fitting.time_datafit_compound_LevMarLSQFitter        |
2024-07-11T16:08:05.5972159Z | -        | 2.17±0.03ms          | 968±7μs             |    0.45 | modeling.fitting.time_Gaussian1D_LevMarLSQFitter              |
2024-07-11T16:08:05.5973944Z | -        | 368±60μs             | 162±3μs             |    0.44 | units.time_quantity_array_conversion                          |
2024-07-11T16:08:05.5975459Z | -        | 289±70μs             | 92.4±0.7μs          |    0.32 | units.time_quantity_init_array                                |
2024-07-11T16:08:05.5977071Z | -        | 260±2ms              | 64.9±0.3ms          |    0.25 | modeling.fitting.time_large_gauss_combined_1d_LevMarLSQFitter |
2024-07-11T16:08:05.5978602Z | -        | 19.9±4ms             | 4.22±0.02ms         |    0.21 | modeling.fitting.time_combined_gauss_1d_LevMarLSQFitter       |
2024-07-11T16:08:05.5980178Z | -        | 176±30μs             | 35.8±0.2μs          |    0.2  | units.time_quantity_creation                                  |
2024-07-11T16:08:05.5981557Z | Change   | Before [ff59e9cf]    | After [2a05840e]    |   Ratio | Benchmark (Parameter)          |
2024-07-11T16:08:05.5982920Z |----------|----------------------|---------------------|---------|--------------------------------|
2024-07-11T16:08:05.5984049Z | +        | 41.8±0.1μs           | 83.2±40μs           |    1.99 | units.time_quantity_times_unit |
2024-07-11T16:08:05.5985127Z 
2024-07-11T16:08:05.5985548Z SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

astrofrog · 2024-07-15T16:15:45Z

The fitting results are all genuine and reliable (they are on the >1ms level). Not sure what's up with the units results but they are on the microsecond level so maybe just can't measure that in CI?

nden

LGTM

pllim · 2024-08-10T13:13:22Z

Does this comment need to be resolved? 👉 #16673 (comment)

astrofrog · 2024-08-10T13:58:07Z

No I was just commenting that the units asv tests were unreliable but not related to the changes here. This is good to go.

mcara · 2024-09-09T23:38:28Z

This PR is breaking code that relies on documented behavior, specifically: https://docs.astropy.org/en/stable/api/astropy.modeling.fitting.LevMarLSQFitter.html#astropy.modeling.fitting.LevMarLSQFitter.objective_function

It introduces additional parameters to args that are not expected. This is a change in API (maybe not formally but still, changing arguments provided to a callback has similar effects).

pllim · 2024-09-10T02:05:37Z

Oh no! We should either

address the API change and document it, or
revert this PR, see Revert "Improve fitting performance of most non-linear fitters" #16979

astrofrog · 2024-09-10T06:26:35Z

I have an idea to fix this, can take a look this morning unless @Cadair beats me to it.

neutrinoceros · 2024-09-10T07:22:24Z

Thanks for the heads up @astrofrog ! I was about to give it a try myself, glad we could avoid another race condition :)

pllim · 2024-09-10T17:19:30Z

Is #16982 @Cadair beating @astrofrog to it?

Cadair · 2024-09-10T17:25:35Z

Yes

Add a changelog about the API change to objective_function in #16673

Cadair added modeling Performance benchmark Run benchmarks for a PR labels Jul 4, 2024

Cadair force-pushed the improve-modeling-performance branch 2 times, most recently from f9855e9 to f7cd5ec Compare July 4, 2024 13:39

astrofrog mentioned this pull request Jul 4, 2024

Add has_tied, has_fixed and has_bounds properties to Model #16677

Merged

1 task

pllim added this to the v7.0.0 milestone Jul 5, 2024

Cadair force-pushed the improve-modeling-performance branch 2 times, most recently from b98648e to 65c65c7 Compare July 10, 2024 15:43

Cadair marked this pull request as ready for review July 10, 2024 15:44

Cadair requested a review from a team as a code owner July 10, 2024 15:44

astrofrog reviewed Jul 11, 2024

View reviewed changes

docs/changes/modeling/16673.feature.rst Show resolved Hide resolved

astrofrog mentioned this pull request Jul 11, 2024

Add function to carry out parallel fitting on N-dimensional datasets #16696

Merged

9 tasks

Cadair and others added 4 commits July 11, 2024 16:34

Speed up non-linear fitters by evaluating models using .evaluate insi…

2c514d0

…de the objective function and avoiding modifying a model instance at every iteration.

Pass some static context through to the objective function

7e54f37

Fix use of wrap_deriv now that we pass the context as an extra argument

9171788

Add changelog

390780d

Cadair force-pushed the improve-modeling-performance branch from f02719c to 390780d Compare July 11, 2024 15:34

nden approved these changes Aug 10, 2024

View reviewed changes

nden merged commit f788114 into astropy:main Aug 10, 2024

mcara mentioned this pull request Sep 10, 2024

Update test matrix spacetelescope/tweakwcs#207

Merged

pllim mentioned this pull request Sep 10, 2024

Revert "Improve fitting performance of most non-linear fitters" #16979

Closed

Cadair mentioned this pull request Sep 10, 2024

Fix _NoneLinearLSQFitter having a required extra arg #16982

Merged

1 task

This was referenced Sep 27, 2024

BUG: TypeError: _LevMarLSQFitter2x2.objective_function() got an unexpected keyword argument 'fit_param_indices' #17085

Closed

Add a changelog about the API change to objective_function in #16673 #17101

Merged

pllim added a commit that referenced this pull request Oct 17, 2024

Merge pull request #17101 from Cadair/modeling_breaking_change

cfbd35d

Add a changelog about the API change to objective_function in #16673

neutrinoceros mentioned this pull request Oct 22, 2024

Small regression in model fitting when using multi-dimensional parameters #17233

Closed

astrofrog mentioned this pull request Oct 30, 2024

Added what's new entry for modeling speedups #17272

Merged

1 task

Uh oh!

Conversation

Cadair commented Jul 4, 2024 • edited by astrofrog Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 4, 2024

Uh oh!

github-actions bot commented Jul 4, 2024

Uh oh!

Cadair commented Jul 4, 2024

Uh oh!

astrofrog commented Jul 4, 2024

Uh oh!

Uh oh!

pllim commented Jul 11, 2024

Uh oh!

astrofrog commented Jul 11, 2024

Uh oh!

pllim commented Jul 11, 2024

Uh oh!

Cadair commented Jul 11, 2024

Uh oh!

astrofrog commented Jul 15, 2024

Uh oh!

astrofrog commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Jul 15, 2024

Uh oh!

nden left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Aug 10, 2024

Uh oh!

astrofrog commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcara commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Sep 10, 2024

Uh oh!

pllim commented Sep 10, 2024

Uh oh!

Cadair commented Sep 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Cadair commented Jul 4, 2024 •

edited by astrofrog

Loading

astrofrog commented Jul 15, 2024 •

edited

Loading

astrofrog commented Aug 10, 2024 •

edited

Loading

mcara commented Sep 9, 2024 •

edited

Loading

pllim commented Sep 10, 2024 •

edited

Loading

astrofrog commented Sep 10, 2024 •

edited

Loading