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

Add background model for map fit #1990

Merged
merged 9 commits into from Jan 22, 2019
Merged

Add background model for map fit #1990

merged 9 commits into from Jan 22, 2019

Conversation

@AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Jan 18, 2019

In progress now.

@adonath adonath self-assigned this Jan 18, 2019
@adonath adonath self-requested a review Jan 18, 2019
@adonath adonath added the feature label Jan 18, 2019
@adonath adonath added this to In progress in gammapy.modeling via automation Jan 18, 2019
@adonath adonath added this to the 0.10 milestone Jan 18, 2019
@adonath
Copy link
Member

@adonath adonath commented Jan 18, 2019

As discussed in our call, please just remove the covariance dispatching in https://github.com/gammapy/gammapy/blob/master/gammapy/cube/models.py#L154 and https://github.com/gammapy/gammapy/blob/master/gammapy/cube/models.py#L165. And set the covariance on the spectral models in the analysis_3d notebook "by hand". I'll do a final review. once the remaining changes are in.

AtreyeeS added 2 commits Jan 21, 2019
@AtreyeeS
Copy link
Member Author

@AtreyeeS AtreyeeS commented Jan 21, 2019

This PR adds the implementation of background models in gammapy, specially addressing (#1804).
This also solves the issue of getting nans in the covariance matrix as pointed out in #1868

Copy link
Member

@adonath adonath left a comment

Thanks @AtreyeeS! I've left a few in-line comments and maybe you could still address #1990 (comment). Once this is done the PR is ready to merge...

@@ -232,6 +256,7 @@ def apply_edisp(self, npred):
npred.data = data
return npred

@property

This comment has been minimized.

@adonath

adonath Jan 21, 2019
Member

Note this change is not backwards compatible. Use will have to adapt their scripts or they'll get errors. I'd prefer to not make this an attribute and just keep the old behaviour. We will make the larger API changes once we introduce the datasets...

err_amp = np.sqrt(covariance[4][5])
err_lon_0 = np.sqrt(covariance[0][1])
err_ind = np.sqrt(covariance[3][4])

pars = result.model.parameters

This comment has been minimized.

@adonath

adonath Jan 21, 2019
Member

I you replace pars = result.model.parameters with pars = fit.evaluator.parameters here, the nicer pars.error() interface works again. I'd prefer to not compute the errors again in the test...(see comment above)

assert_allclose(npred, 2455.230889, rtol=1e-3)
assert_allclose(result.total_stat, 5417.350078, rtol=1e-3)

covariance = fit.evaluator.parameters.covariance_to_table()
err_amp = np.sqrt(covariance[4][5])

This comment has been minimized.

@adonath

adonath Jan 21, 2019
Member

This should not be needed. Once the covariance is available on fit.evaluator.parameters, the .error() access should work...

background=None,
psf=None,
edisp=None,
back_model=None,

This comment has been minimized.

@adonath

adonath Jan 21, 2019
Member

Could you rename this to background_model?

@AtreyeeS
Copy link
Member Author

@AtreyeeS AtreyeeS commented Jan 21, 2019

Thanks @adonath! I have made the changes as you commented

@adonath
Copy link
Member

@adonath adonath commented Jan 22, 2019

@AtreyeeS Thanks, I just pushed a few minor clean-up commits. Now the PR is ready to merge...

@adonath adonath merged commit 038bbb9 into gammapy:master Jan 22, 2019
2 of 4 checks passed
2 of 4 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Scrutinizer Analysis: 2 updated code elements – Tests: passed
Details
gammapy.gammapy Build #20190122.3 succeeded
Details
gammapy.modeling automation moved this from In progress to Done Jan 22, 2019
@cdeil cdeil changed the title Adding a background model fit Add background model for map fit Jan 23, 2019
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