Repository navigation
BUG: TypeError: _LevMarLSQFitter2x2.objective_function() got an unexpected keyword argument 'fit_param_indices' #17085
Description
As reported by @mcara at #16982 (comment) , #16982 to fix backward compatibility that #16673 broke was incomplete fix. More fix is needed.
TypeError: _LevMarLSQFitter2x2.objective_function() got an unexpected keyword argument 'fit_param_indices'
Activity
astrofrog commentedon Sep 30, 2024
Just thinking out loud, I feel like a lot of things on fitters are public and shouldn't really be. Shouldn't the only public method on fitters be
__call__? I wonder if a solution would be that the base classes actually implement_objective_functionand use it and this would support the additional kwargs and then check if objective_function is defined on the function and call it without the kwarg if so, but potentially emit a deprecation warning aboutobjective_function?Alternatively, we could check whether objective_function has be overriden and if so not pass the extra arguments if so?
Cadair commentedon Sep 30, 2024
I like the idea of migrating to
_objective_function, I am not sure I see much in the way of legitimate use of customising that method when what you should be fitting is the model.I think the thing here is the difference between the user API and the developer API for people who want to write their own fitters outside of core? I think that the developer API for this fitting module is shockingly bad and ill documented. The class hierarchy here is a mess and I think it needs a cleanup.
In this case the error was caused by us passing in an kwarg to a function not expecting it: https://github.com/spacetelescope/tweakwcs/pull/212/files#diff-fb9a903d102a166923773794afe09730d72c88f416b641c178b2c213c6986aa5R853
I don't know how to handle supporting people writing their own fitters, and actually making forward progress on performance here. I do not want to start passing around fitting state on the class itself just to prevent any potential API breakage for people writing their own fitters as subclasses of
_NonLinearLSQFitter(or it's public subclasses).My preferred solution to this would be to add a breaking changelog entry and move onto cleaning up the whole mess at a later date.
mcara commentedon Sep 30, 2024
Does "...customising that method..." refer to
_objective_functionorobjective_function? I think there are legitimate uses when users want to minimize a different measure from what current fitters offer. Maybe I am mis-understanding the issue (sorry, I have not been following closely the fitting module and recent PRs).Looking back: #11736 would be an example. And following links in the issues, I get to #11735 which could be related to this discussion.
I actually do not find the user-facing documentation that bad, i.e.,
https://docs.astropy.org/en/stable/modeling/new-fitter.html
Not being a maintainer for this module, it is hard for me to evaluate consequences for this. However, I just find hard to believe that subclassing fitters is not needed at all. If that is the case, why are fitters classes at all and not functions?
CC: @WilliamJamieson
astrofrog commentedon Sep 30, 2024
I think the preferred API that we should be striving towards is that of the Fitter class where users can specify optimisers and statistics.
astrofrog commentedon Sep 30, 2024
(But the current non linear fitters need to be migrated to that API)
Cadair commentedon Oct 1, 2024
I have opened a PR with a changelog entry calling out the API change for people who are subclassing one of the non-linear fitters.
I think that you are right @mcara the public API for subclassing
Fitteris pretty well documented and should obviously be supported for custom things. What we are talking about here though is a breakage for people subclassing one of the existing non-linear fitter classes, which are a lot more complex.As @astrofrog says, I think the whole module could do with a API cleanup to make everything clear, but I don't expect many people will be doing what you are doing and subclassing an existing non-linear fitter to adjust the objective_function? Maybe they will who knows.
I am open to better options here, but I also think this small API change is worth the significant performance increase it enables.
As a sidenote @mcara you can also make use of the new kwarg in your fitter to get a nice perf boost, by passing it through to
fitter_to_model_params.