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

Improve Fit class, add confidence method #1927

Merged
merged 3 commits into from Nov 15, 2018
Merged

Improve Fit class, add confidence method #1927

merged 3 commits into from Nov 15, 2018

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 15, 2018

This PR improves the Fit class in gammapy.utils.fitting a bit, doing some polishing.
It also adds a new feature Fit.covariance as a new feature, wrapping Minuit.minos.
Seems to work OK for one simple example, see test_confidence

I also made a small change to the Parameters class, so that one can access parameters not just by name and number, but also to pass a Parameter object in.
This is what Sherpa does, users can pass in Parameter objects, not just names, see e.g. https://sherpa.readthedocs.io/en/latest/quick.html#a-single-parameter .
I'd like to evolve the Fit API more i this direction, because if you have a double-Gaussian, then you currently can't reach the second parameter by name. Maybe that globally unique name can be achieved, maybe not, but working with Parameter objects more in the API seems like a nice solution to me.

@adonath - Please review, especially if you're OK to have the sigma option in confidence, or if you want or need some other API, like being able to select a TS difference to reach instead. Did we make a decision there in the discussion on API to select confidence intervals?
Note that both MINUIT and Sherpa use an option "sigma" here:
https://sherpa.readthedocs.io/en/latest/fit/index.html#changing-the-error-bounds
So I think I'm +1 to also go that route, but I'm not sure. Especially the whole question of one vs two-sided is a bit more unintuitive with "sigma" ,no?

@cdeil cdeil added the feature label Nov 15, 2018
@cdeil cdeil added this to the 0.9 milestone Nov 15, 2018
@cdeil cdeil self-assigned this Nov 15, 2018
@cdeil cdeil added this to In progress in gammapy.modeling via automation Nov 15, 2018
@cdeil cdeil requested a review from adonath Nov 15, 2018
Copy link
Member

@adonath adonath left a comment

Thanks, @cdeil! I've left a few minor in-line comments. Let's do the discussion about, when to return a dict or a new Result object face to face.

# set amplitude of fit model to best fit amplitude
parameters["amplitude"].value = amplitude
covar_opts.setdefault("backend", "minuit")
if covar_opts["backend"] in registry.register["covar"]:

This comment has been minimized.

@adonath

adonath Nov 15, 2018
Member

I think this logic is at the wrong place, no? I think it should mainly reduce to the one line covar_result = self.covar(**covar_opts), the rest should be handled either in Fit.covar or the Registry. (like raising an error if a backend does not support covar). There is no need to set "covar_opts.setdefault("backend", "minuit")" either, as it is already set as a default in Fit.covar.

This comment has been minimized.

@cdeil

cdeil Nov 15, 2018
Author Member

The registry always gives an error. Here I want to emit a warning and continue.
I've changed this a bit, using a simpler if clause, which I prefer over try-except in this case.

def run(self, optimize_opts=None, covar_opts=None):
def confidence(self, parameter, backend="minuit", sigma=1, maxcall=0):
"""Estimate confidence interval.

This comment has been minimized.

@adonath

adonath Nov 15, 2018
Member

Add section Parameters to the docstring?

This comment has been minimized.

@cdeil

cdeil Nov 15, 2018
Author Member

Done

else:
raise NotImplementedError()

def likelihood_profiles(self, model, parameters="all"):

This comment has been minimized.

@adonath

adonath Nov 15, 2018
Member

As I said already in private communication, I'd be fine to remove this method, as it can be achieved by users with a few lines of Python.

This comment has been minimized.

@cdeil

cdeil Nov 15, 2018
Author Member

done

@@ -207,59 +172,130 @@ def covar(self, backend="minuit"):
result : `CovarResult`

This comment has been minimized.

@adonath

adonath Nov 15, 2018
Member

Add section Parameters to the Fit.covar docstring?

This comment has been minimized.

@cdeil

cdeil Nov 15, 2018
Author Member

done

@cdeil cdeil changed the title Improve gammapy.utils.fitting Improve Fit class, add confidence method Nov 15, 2018
@cdeil cdeil merged commit 73ba34e into gammapy:master Nov 15, 2018
0 of 5 checks passed
0 of 5 checks passed
gammapy.gammapy Build #20181115.6 has failed
Details
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Installing Code
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
gammapy.modeling automation moved this from In progress to Done Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants