Add a fit method to models

[astrofrog]

I haven't checked through all the open modeling issues, but I think model classes should have a fit method (that would default to one of the fitters, but could also take an argument for the fitter to use). I think the following 'beginner' example could be made simpler if it looked like:

>>> g1 = models.Gaussian1DModel(10., stddev=2.1, mean=4.2)
>>> g1.fit(x, y)

[embray]

I like this idea. The default fitter to use could even be defined on a per-model basis (though also overridable).

[cdeil]

I'm not sure this is a good idea to select one of the Fitters as a default and different fitters have different input data, so the signature of this method would be complex.

Can we hold off deciding whether to add this until we have a few more fitters in astropy?
(I will do #1056 in the next days)

[astrofrog]

@cdeil - then we could consider at least supporting the most common fitting algorithm.

[astrofrog]

Having just played around with modeling some more this morning, what I find unintuitive is that if one has a model m and a fitter associated with that model, doing:

fit(x, y)

changes the state of m, which doesn't feel right. I would have thought that:

m.fit(x,y)

would change the state of m, but:

fit(x,y)

would just return a dictionary of parameters or something similar.

[nden]

This looks like one of those features which is easy to add and would be hard to remove or change. And I think developing a flexible way to construct a fitter with user-defined optimization method and statistic (and eventually error estimation method) should be done before adding a convenience method m.fit() since this will change the signature of the method. So I would push this for after 0.3 unless there's a strong opinion that it is needed now.

But to be sure I understand what you have in mind, is this how you see it working:

>>> m = models.Gaussian1DModel(10., stddev=2.1, mean=4.2)
>>> print(m.default_fitter)
'SLSQPFitter'
>>>print(m.default_statistic)
'leastsq'
>>> m.fit(fitter='NonLinearLSQFitter`, statistic='Cash')

Also, since I see m.fit() as a convenience, I would prefer fitter(x, y) to return a model which is ready to be evaluated, as this makes it more flexible and easier in the non-default case and in the case of a custom model.

It may be good to get other opinions on this @eteq @keflavich Anyone else?

[astrofrog]

I agree the fit method can wait until later. For now, what bugs me is that fitter(x,y) changes m but there is no mention of m in that line. What about (I think you are suggesting something similar):

m_fitted = fitter(m, x, y)

i.e. m is the original model, m_fitted is the model with the fitted parameters, then everything is explicit and one can re-use the same model as initial conditions to fit multiple datasets.

(it's also conceptually simpler that the fitter takes the model and the data as inputs)

[nden]

I agree this behaviour can be a bit unexpected for users and returning a copy of the modified model may be better. Of course one has to be careful about reusing the initial values with different data sets.

I'm not so sure about passing the model to fitter.__call__. Currently the fitting is a two step process:

fitter = fitting.NonLinearLSQFitter(model)
fitter(x, y)

and the state of the fitter is set in the first step. The most important part of this is determining the constraints and which parameters are supposed to be fit (frozen and tied parameters are not fit).
It's good to keep the __call__ method with as little overhead as possible (although I haven't actually timed a fitter with a model passed to __call__ and setting the state there..

One way around this may be to have a model.initial_parameters list and model._initialize() method which fitter.__call__ calls before actually doing the fit. So the syntax will be:

fitter = fitting.NonLinearLSQFitter(model)
m_fitted = fitter(x, y)

A modified copy of the model is returned and the model is initialized before each fitting.

I understand this is still not as explicit as fitter(m, x, y). Do you think this would be sufficient? If not I can play with how fitters work now and see the performance of something like:

fitter = fitting.NonLinearLSQFitter(l)
m_fitted = fitter(model, x, y)

[eteq]

delaying for the next version is fine - I see the point that it's more important to get it right the first time, and the "create a new model" paradigm does sound safer

[eteq]

@astrofrog @nden @embray @cdeil - is this now "resolved" in that we've accepted the current system of new_model = fitter(initial_model, x, y), or should it be left open?

[astrofrog]

Maybe we can simply close this and see if anyone else raises this in future?

[embray]

I'm -0 on closing it.

This is definitely not a priority right now. But it's not an altogether bad idea either. Putting this in "Future"

[pllim]

Is this a duplicate of #979?

[astrofrog]

No I think they are a bit different - this is saying models should have a fit method, whereas #979 (I think) is saying there should be a reference to the most recent fitter used.

[astrofrog]

Let's close this - we have way too many issues of 'let's keep open just in case' these days, and I agree with @eteq that we've settled on an accepted way to use the fitters.