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
No copy of input and result model in fit #1937
Conversation
I tried to remove the model copies in dace318 . The Basically - there's something we have to change how especially the Minuit backend works and interacts with the E.g. we could introduce a @adonath - Maybe we can do some pair-coding to try and finish this up tomorrow? |
@cdeil Even if we remove the model copy, I think the parameters should be copied before running |
Ah, I just realized this does not work, because once we decouple the parameters from the model, the model evaluation is not possible any more. I think we have to re-discuss the issue... |
@adonath - Let's discuss later today. I'm OK to spend another day or two on this, it's important to find a good solution here. |
@adonath and I discussed this question of model copies again. It's a difficult decision which way to go. We checked how Sherpa works in detail here - they have a two-level On the other hand, we checked that Astropy does make a model copy on start in the fit call. So that's a valid way to go also, and they also have complex parameter linking and compound models. But they don't have caching, and they only have a single Given that state of understanding, I'm +1 to continue with this PR and to not make copies. I also prefer it as a user in notebooks or ipython or scripts, I find it easier to have one model and do several operations with it (like freeze parameters, fit the rest, unfreeze, fit again, ...), and to explicitly call copy() only if needed. @adonath is 50:50, could go either way. The way that sherpa avoids changing the best-fit parameters e.g. in @registerrier @adonath @AtreyeeS and anyone interested - please keep thinking about how Gammapy models should work, especially in the context of multiple datasets and background models, and then write a PIG for this soon and we try to prototype that ASAP. My guess is that the global model solution a la Sherpa will be complex and at times problematic and error-prone, but effectively still the best and the route we choose, because making copies all the time is worse. |
@adonath - Please review. |
@cdeil I added a |
@adonath - Wonderful, thanks! |
As discussed previously (see #1783 (comment) and Gammapy call last week), @adonath and I propose to avoid making model copies.
At the moment there are three separate copies:
fit._model
fit.result.model
Clearly this has great potential to confuse users, the more model copies the more complex.
The reason the model copies were introduced was because
fit.profile
or manually modifying state onfit.minuit
resulted in changes to the input and results model previously.fit.profile
has already been changed to make a copy of the parameters object on enter, it no longer has this issue. Other methods like contour or confidence interval computation should work the same. Soon there will be no more need to accessfit.minuit
directly, and if someone does she's an expert and can callmymodel.copy()
herself.So this PR removes all the copying.
Given that parameter values and covar are currrently stored in
model.parameters
, the model owns that state, and the fit methods modify it in place. Whether we still want to returnresult.model
at all can be discussed later. For now I keep it for backward compatibiltiy. It's just another reference to the one model that was passed in, users can access results from either the input model orresults.model
. Thefit._model
will probably be replaced with something else, @adonath will make a proposal or PIG concerning a better design of thefit
class.For now, I have added asserts to the tests for fit classes we have (
Fit
,MapFit
,SpectrumFit
andFluxPointFit
) that establish the copy behaviour, as expected copies are currently made. I'll add one more commit here to actually change the behaviour, and adapt tests and docs tomorrow.cc @registerrier @adonath @AtreyeeS