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

Merged
merged 5 commits into from Nov 22, 2018

Conversation

Projects
2 participants
@cdeil
Member

cdeil commented Nov 21, 2018

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:

  • User defined input model
  • 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 on fit.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 access fit.minuit directly, and if someone does she's an expert and can call mymodel.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 return result.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 or results.model. The fit._model will probably be replaced with something else, @adonath will make a proposal or PIG concerning a better design of the fit class.

For now, I have added asserts to the tests for fit classes we have (Fit, MapFit, SpectrumFit and FluxPointFit) 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

@cdeil cdeil added this to the 0.9 milestone Nov 21, 2018

@cdeil cdeil self-assigned this Nov 21, 2018

@cdeil cdeil added this to In progress in Modeling via automation Nov 21, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 21, 2018

I tried to remove the model copies in dace318 .

The test_confidence in test_fit.py is failing - I didn't manage to avoid the state change there in the iminuit backend yet. The problem is that the MinuitLikelihood object is created in optimize_iminuit, but then only it's fcn is passed to Minuit, but there is still the reference to the parameters that were used in optimize. So I don't see a way to avoid modifying those in confidence_iminuit.

Basically - there's something we have to change how especially the Minuit backend works and interacts with the Fit class.

E.g. we could introduce a MinuitState object in optimize and also return the MinuitLikelihood instance, so that we still can access and make copies of Parameters as needed in confidence. Or we could try and see if we can get minos to execute with a new Minuit instance to have a stateless backend for that part.

@adonath - Maybe we can do some pair-coding to try and finish this up tomorrow?

@adonath

This comment has been minimized.

Member

adonath commented Nov 22, 2018

@cdeil Even if we remove the model copy, I think the parameters should be copied before running Fit.covariance(), Fit.confidence() or Fit.likelihood_profile(), to avoid that the model parameters remain in an undefined state, after the algorithm has run. So I think for consistency the same should happen in Fit.optimize(), which would also resolve the current issue with the MinuitLikelihood().

@adonath

This comment has been minimized.

Member

adonath commented Nov 22, 2018

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...

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 22, 2018

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

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 22, 2018

@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 __call__ and calc interface in model evaluation, and in principle could be able to evaluate models without changing parameter state, at least for simple models. But they do change model parameter values during optimisation and and covar and any other computation, never make a copy of Parameter or Model objects. My guess is that they either found that solution simpler, or it's just required for complex connected models, e.g. they support callable-defined parameter linking, and things would horribly break or be very inefficient when starting to make copies. They also have caching decorators on model calc functions, presumably the way their caching works would also not be possible with copies.

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 fit(), not things like covar or confidence or profile computation, i.e. not really a fitting framework.

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 fit.est_errors() is to save state in local variables (numbers and lists, not Parameter or Model objects, those are never copied) in methods like est_errors, and to restore that state as needed at the end of those methods. That's pretty error-prone and annoying, but clearly feasible. So that's what I'll also do here for now.

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

@cdeil cdeil force-pushed the cdeil:rm-model-cp branch from dace318 to 73bcd1b Nov 22, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 22, 2018

@adonath - Please review.

@adonath

This comment has been minimized.

Member

adonath commented Nov 22, 2018

@cdeil I added a Parameters.restore_values context manager and removed the print statements. I'm not sure whether it's a common pattern to have a context manager attached to a class, but I found it a better place then in utils/fitting/fit.py. I'll go ahead an merge now.

@adonath adonath merged commit 7244de4 into gammapy:master Nov 22, 2018

1 of 4 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Scrutinizer Analysis: 2962 new issues, 6 updated code elements – Tests: passed
Details

Modeling automation moved this from In progress to Done Nov 22, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 22, 2018

@adonath - Wonderful, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment