Deprecate use of LMLSQFitter for models with bounds - #16994

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.

The main reason the CI is failing in a number of places is because Gaussian1D uses bounds on stddev by default, and Gaussian1D is used as the de-facto model in tests. There are three main options at this point:

1. Don't run the tests that use Gaussian1D with LMLSQFitter
2. Re-write many tests to use a different model that doesn't have bounds
3. Find another way to deal with restricting stddev to positive values on Gaussian1D that doesn't involve bounds

For option 3, what's interesting is that for instance Lorentz1D doesn't have bounds on the fwhm and we've never had any complaints. In fact, the equation of the gaussian works fine with negative sigma so this is more just about what users would expect, as they would find a negative sigma strange (even though it would be mathematically valid). So I wonder if we can find a different way to handle this. I did try a few things and ran into other issues.

Option 1 is the easiest, so it might make the most sense for now (this is what's I'll implement here for now)

I think the CI should be fixed now (I also think we need fixtures to avoid repeating the list of fitters in all test files, but that can wait)

but that can wait

You mean wait to merge until you have time for it here? Or open follow up issue? Hope you can clarify. Thanks!

I wonder how many downstream packages will be affected by this deprecation warning... 👀

I wonder how many downstream packages will be affected by this deprecation warning... 👀

I would hope that not too many people are using this new LMSQ fitter when the ones with good bounds support were added at the same time. If people are using this I would assume they are doing it because they don't understand that they would probably be better off using one of the other ones.

This definitely needs to be reviewed by @nden or @perrygreenfield as this is not a trivial change.

From searching code on GitHub I found a handful (perhaps a dozen or a little more) usages of LMLSQFitter with e.g. Gaussian models, so those will definitely receive deprecation warnings. So it will definitely impact some users, though I don't think it's very widespread as many many users will copy examples from docs which use LevMarLSQFitter. The users it does affect will get warnings and will have at least a year to adjust to using a different fitter.

Just for full disclosure, we could also still optionally support the current bounds algorithm via a use_min_max_bounds kwarg in the initializer in case users do want to opt in to it, but I think it just boils down to whether we still need to do this given that other fitters support bounds properly. If we don't support bounds at all in LMLSQFitter, we'll be doing the same as scipy's least_squares, which makes users use other algorithms for bounds support (which I think is reasonable).

I strongly think supporting use_min_max_bounds is a bad idea for anything but legacy use in LevMarLSQFitter.

@astrofrog FYI, this PR causes a photutils test to fail.

photutils PSF photometry accepts a fitter_maxiters keyword that is then passed on as maxiter to fitters that support it. It does this by inspecting the fitter __call__ function signature. By overriding the LMLSQFitter __call__ signature to simply __call__(self, model, *args, **kwargs) the maxiter keyword no longer explicitly shows up and a new photutils warning is raised, causing a test to fail. The _NLLSQFitter base class __call__ method explicitly has a maxiter keyword.

Does it make sense to move maxiter to the fitter __init__ method? But that would be a bigger breaking change.....

Uh oh... should we revert this?

To prevent the breaking API change, all that would be needed is to update the LMLSQFitter.__call__ signature to match the base class. I can open a PR to do that if you want.

Yes that would be great, thanks!

The new __call__ method in this PR also does not have the @fitter_unit_support decorator. That is needed, right?