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 FluxPointEstimator for joint likelihood datasets #2152

Merged
merged 11 commits into from May 23, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented May 22, 2019

This PR includes the following changes:

  • Add FluxPointsEstimator.estimate_counts() method, that computes how many counts (per observation) actually contribute to the measurement. The info is also stored in the output table.
  • Change back to the upper limit computation with iminuit, but use the ccipy backend when there are zero counts in the energy bin.
  • Freeze the parameters of the background model if an observation has zero counts, that contribute to the measurement
@adonath adonath self-assigned this May 22, 2019
@adonath adonath added this to the 0.12 milestone May 22, 2019
@adonath adonath requested a review from lmohrmann May 22, 2019
Copy link
Contributor

@lmohrmann lmohrmann left a comment

Looks mostly good to me, I left a few inline comments. Thanks!

gammapy/spectrum/flux_point.py Show resolved Hide resolved
gammapy/spectrum/flux_point.py Outdated Show resolved Hide resolved
gammapy/spectrum/flux_point.py Outdated Show resolved Hide resolved
Copy link
Contributor

@registerrier registerrier left a comment

Thanks @adonath . Looks good! I have a few inline comments.


actual = fp.table["norm"].data
assert_allclose(actual, [0.97922, 0.94081, 1.074426], rtol=1e-3)

actual = fp.table["norm_err"].data
assert_allclose(actual, [0.069966, 0.052617, 0.092854], rtol=1e-2)

actual = fp.table["counts"].data
assert_allclose(actual, [[44445, 0], [1911, 0], [292, 0]], rtol=1e-2)
Copy link
Contributor

@registerrier registerrier May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we need a tolerance for counts here?

Copy link
Member Author

@adonath adonath May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -106,19 +109,22 @@ def test_str(fpe_pwl):
def test_run_pwl(fpe_pwl):
fp = fpe_pwl.run()
actual = fp.table["norm"].data
assert_allclose(actual, [1.080933, 0.910776, 0.922278], rtol=1e-5)
assert_allclose(actual, [1.081434, 0.91077 , 0.922176], rtol=1e-5)
Copy link
Contributor

@registerrier registerrier May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't a relative tolerance of 1e-5 too strict for minimization results?

Copy link
Member Author

@adonath adonath May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree rtol=1e-2 should be sufficient.

return [par.frozen for par in self.parameters]

@frozen.setter
def frozen(self, value):
Copy link
Contributor

@registerrier registerrier May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior is a bit strange no? This will freeze or thaw all parameters at once. If the property returns an array or a list, I would assume the setter to expect one as well.

Wouldn't it be more clear to have a freeze_all or thaw_allmethods?

Copy link
Member Author

@adonath adonath May 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a good point. I didn't realise the getter and setter approach was inconsistent. I'll implement you proposal with the freeze_all() method, which is the only one I need in this PR.

@adonath adonath force-pushed the fpe_improvements branch from a57e65f to f05125d May 23, 2019
@adonath adonath force-pushed the fpe_improvements branch from f05125d to 8e80371 May 23, 2019
@adonath adonath merged commit bea9b02 into gammapy:master May 23, 2019
9 checks passed
@adonath adonath changed the title Improvements to FluxPointEstimator Improve FluxPointEstimator for joint likelihood datasets May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants