Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fitters return a copy of the model #1564

Closed
wants to merge 4 commits into from
Closed

Conversation

nden
Copy link
Contributor

@nden nden commented Oct 12, 2013

As discussed in #1330 and #979 fitters should return a modified copy of the model instead of modifying it in place.
This PR is not completed yet but I am submitting it to get opinions on a side effect which breaks the current constraints API.

As a reminder of how the constraints code evolved, initially constraints were attributes on fitters but it was felt largely that it would be better to move them to models and in particular be attributes on parameters (~astropy.modeling.parameters.Parameter) which is what they currently are. This facilitates interactive use of ~astropy.modeling, so it's possible to do something like:

gauss = models.Gaussian1DModel(amplitude=10, mean=3, stddev=1)
gauss.stddev.min = 1
gfit = fitting.NonLinearLSQFitter(gauss)
gfit(x, y)
gauss.stddev.min = 2
gfit(x,y)

The second call to gfit will pick up the change in the constraints.

In this PR fitters make a copy of the model and don't hold a reference to the initial model any more so the above code does not work as expected. This means that changing the constraints interactively will have to use the copy of the model too.

gfit.model.stddev.min = 2

Since this is an end user API change I wanted to see how others felt about it.

@astrofrog
Copy link
Member

I will bring this up for discussion at the telecon today.

@nden
Copy link
Contributor Author

nden commented Oct 30, 2013

Closing this in favor of #1330 and #1702

@nden nden closed this Oct 30, 2013
@nden nden deleted the copy_model branch December 27, 2018 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants