Store fitter object in model object

@nden

In models, Fitters and their Models are separate classes. In the current version, if a Fitter is applied to a model, the Model's parameters are updated, but after that (I think), the Fitter is not referenced from the Model. It might be good for the Model to store a reference to the most recently-used Fitter so that a user can later go back and figure out how that Model got its parameters set to what they are. This might also involve storing the data used by the Fitter.

@nden, what do you think?

(This refers to the models subpackage that will be in astropy when #493 is merged. The intent is to merge that without significant design changes, and address proposed updates through additional issues/PRs like this one)

@eteq

@eteq @nden @embray everyone: This sounds like a fundamental design decision about how Fitter and Model relate. Any thoughts as to why or why not we would want to do this?

It doesn't seem too unreasonable to me. Fitter instances should still be untethered from a specific Model, but if a model got its parameters using a specific fitter it seems relatively harmless to me for the model to store a reference its last-used fitter as an attribute (it doesn't need to be a circular reference, in other words).

Having a last_fitter attribute seems like a useful information. How much do we want to store? Is the name of the fitter sufficient? Or do we need the entire object? If a reference to the fitter object is stored, then it will be possible to do something like:

model = models.Gaussian1DModel(10,3,1)
gfit = fitting.NonLinearFitter(g1)
gfit(x,y)
model.last_fitter.model.parameters = [0,0,0]

which changes the parameters on the initial model.

It will also be possible to do

model.last_fitter(x1, y1)

which changes the parameters but also starts with initial values set by the last fitting call (not by the user) and may not be what was meant.
I wonder if such flexibility would be confusing.

@eteq

@eteq, everyone: Is there a concrete use case where having the model.last_fitter reference would be helpful?

I guess it would have to involve multiple models and / or multiple fitters and / or repeated use of existing models / fitters to multiple data sets?

I cannot think of a case where having model._last_fitter would simplify a script or interactive fitting session, so my suggestion would be to not add the reference to keep objects as loosely coupled as possible.

If someone comes up with a use case in the future we can always add it, whereas if we notice problems in the future removing it would break user's code.

I played around a bit with a simply model and fitter to see what references they currently have:
https://gist.github.com/cdeil/6183097
Everything about the current behaviour looks good to me, although I have to admit that this example is probably too simple to be useful for this discussion.

I didn't realize that Fitter instances do store a reference to a specific model that they're fitting though now that I think about it that makes sense. But in that case I would make any model.last_fitter a weakref (or perhaps the other way around would make sense?)

@cdeil

@cdeil @nden - the main use case I had in mind is that when working interactively, this is valuable for bookeeping. Imagine the following scenario (not actual modeling code, but just the basic elements):

mod = SomeModel()
fitter = SomeFitter(mod)
fitter2 = SomeOtherFitter(mod)
... some code to generate data...
fitter2(x,y)
... some code to analyze the fit...
fitter(x,y)
... some intervening code complex enough to be distracting...

And now I've forgotten what set the parameters for mod. I might guess it's fitter2, because that's the second one I defined, but that'd be wrong. So it's very valuable to do mod.last_fitter and see it's a SomeFitter, not SomeOtherFitter. This is based on the idea that a "model-centric" view is how most typical science cases work, rather than focusing on the fitter itself, which I think is true.

I could be fine with @embray's idea of a weakref, given this case, although I don't really see why that's necessary - are we afraid of memory leaks here?

Yes. If both the model and fitter are referencing each other it creates a reference cycle. I think Python garbage collector can detect and break simple cycles like that now.

I'm thinking maybe the model attribute on the fitter should be a weakref, because if the model you're fitting is no longer referenced anywhere else I don't think having just the fitter for it is that useful. Or is it?

@eteq

@eteq The use case you describe here is not only confusing but may lead to wrong results. The reason is that currently fitters modify the parameters of the input model in place. So fitter will start with possibly wrong initial values (the result of fitter2).
As discussed in #1330 this kind of confusion will not exist if the fitter returns a copy of the model. Do you think this eliminates the need for a model.last_fitter reference? Such reference cycle may possibly make it more difficult to create a copy of a model.

@nden

@nden @embray - I completely agree my example is confusing and might give wrong results, but it is an example of a use case I am certain will happen if modeling is used interactively (and it will be, if it's successful). A simpler version might just be

mod = SomeModel()
fitter = SomeFitter(mod)
fitter(x,y)
... now a bunch of interactive work, and I also leave for a coffee break and a seminar...

and when I come back I've forgotten what fitter initially set the parameters on mod.

I think the idea is #1330 a good thing, regardless, but I'm not sure it fully addresses this, because even if I changed the last line to newmod = fitter(x,y) I might still forget how newmod got its parameters.

I'm pretty sure the python gc handles this fine, but @nden's point that returning model copies may throw a monkey wrench into it. So how about behavior like this:

any model that is not from a fitter has a fit_by property that always gives None
a new model created via newmod = fitter(x,y) has a fit_by property that will return fitter, but internally it is a weakref to fitter. If the weakref has been gced, fit_by is True. That can be implemented as something like this

@property
def fit_by(self):
    if self._fit_by is False:
        return None
    elif self._fit_by() is None:
        return True
    else:
        return self._fit_by()

if you then make another fit out of newmod, you can just replace the existing _fit_by with a weakref to the new fitter.

Then you can be sure there are new reference cycles, because weakrefs never result in cycles. But then when working in interactive mode, I still get my easy access to the old fitter. Does that address all concerns?

I'm re-scheduling this to 1.0 and we can always decide whether to close later. I don't think we'll reach a resolution in time for 0.4.

Removing the 1.0 milestone since I don't think this is critical

This issue refers to the old implementation of models. After some discussion at PIA hack day, I am going to close it. If someone feels strongly about this please open a new issue.

Navigation Menu

Repository navigation

Store fitter object in model object #979

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Store fitter object in model object #979

Description

Activity

cdeil commented on Aug 7, 2013

embray commented on Aug 7, 2013

nden commented on Aug 7, 2013

cdeil commented on Aug 8, 2013

embray commented on Aug 8, 2013

eteq commented on Aug 14, 2013

embray commented on Aug 14, 2013

nden commented on Oct 11, 2013

eteq commented on Oct 16, 2013

astrofrog commented on May 18, 2014

astrofrog commented on Jan 14, 2015

nden commented on Aug 1, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions