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

Add fit minos_contour method #1949

Merged
merged 4 commits into from Nov 26, 2018

Conversation

Projects
2 participants
@cdeil
Member

cdeil commented Nov 26, 2018

This PR adds Fit.minos_contour that wraps https://iminuit.readthedocs.io/en/latest/api.html#iminuit.Minuit.mncontour

It also contains a commit that fixes the return value of Fit.confidence, to multiply the parameter factor with the scale.

This wasn't noticed because the test case we have is bad. I can change the test case tonight, but would prefer to do it in a separate PR because it'll result in a large diff.

@cdeil cdeil added the feature label Nov 26, 2018

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

@cdeil cdeil requested a review from adonath Nov 26, 2018

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

@adonath

Thanks @cdeil! I've left a few minor, optional in-line comments...

@@ -332,22 +338,33 @@ def likelihood_contour(self):
"""
raise NotImplementedError
def minos_contour(self):
def minos_contour(self, x, y, numpoints=10, sigma=1.0):
"""Compute MINOS contour.

This comment has been minimized.

@adonath

adonath Nov 26, 2018

Member

Maybe add a Parameters section to the docstring? As the signature is the same as Minuit.mncontour, it might not be needed...

This comment has been minimized.

@cdeil

cdeil Nov 26, 2018

Member

OK, I'll add it.

result = mncontour(self.minuit, parameters, x, y, numpoints, sigma)
return {
"is_valid": result["x_info"]["is_valid"] and result["y_info"]["is_valid"],

This comment has been minimized.

@adonath

adonath Nov 26, 2018

Member

I think for the other methods such as .optimize(), .covariance() and confidence() we called the status flag success. Maybe make it consistent?

This comment has been minimized.

@cdeil

cdeil Nov 26, 2018

Member

will do

assert_allclose(len(y), 10)
assert_allclose(y[0], 300, rtol=1e-5)
assert_allclose(y[-1], 300.866004, rtol=1e-5)

This comment has been minimized.

@adonath

adonath Nov 26, 2018

Member

Maybe add one assert on the likelihood value at one of the contour points? But I guess it's not really needed...

This comment has been minimized.

@cdeil

cdeil Nov 26, 2018

Member

I'm not sure if the likelihood at these points is available, or if we should compute it ourselves. Certainly having the stat or ts val of the contour in the return dict would be great.

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 26, 2018

@adonath - Now we have consistently success as flag name.

And I put errp and errn instead of lower and upper for Fit.confidence, to be consistent within Gammapy and the current spec (see here).

@cdeil cdeil merged commit 29253c3 into gammapy:master Nov 26, 2018

0 of 4 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Running
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

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

@cdeil

This comment has been minimized.

Member

cdeil commented Nov 26, 2018

@adonath - I have to go. Merged this in now. If anything needs fixing, feel free to commit to master or make a follow-up PR. I'll try this out for the joint Crab paper tonight or tomorrow morning.

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