Add a fit method to models

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)

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

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)

@cdeil

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

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.

@eteq

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?

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)

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)

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

initial changes for astropy#1330

@astrofrog

@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?

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

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"

Is this a duplicate of #979?

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.

@eteq

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.

Navigation Menu

Repository navigation

Add a `fit` method to models #1330

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add a fit method to models #1330

Description

Activity

embray commented on Aug 8, 2013

cdeil commented on Aug 13, 2013

astrofrog commented on Aug 13, 2013

astrofrog commented on Sep 29, 2013

nden commented on Sep 30, 2013

astrofrog commented on Sep 30, 2013

nden commented on Sep 30, 2013

eteq commented on Oct 16, 2013

eteq commented on May 1, 2014

astrofrog commented on May 1, 2014

embray commented on May 1, 2014

pllim commented on Jun 15, 2017

astrofrog commented on Jun 15, 2017

astrofrog commented on Mar 21, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Add a `fit` method to models #1330