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 Feldman Cousins code #813

Merged
merged 5 commits into from Dec 12, 2016
Merged

Improve Feldman Cousins code #813

merged 5 commits into from Dec 12, 2016

Conversation

@dlennarz
Copy link
Contributor

@dlennarz dlennarz commented Dec 8, 2016

In the calculation of average upper limit, the code now skip bins with low probability. This makes this code more robust and the user does not have to find a good range of x-values. The value for low probability is a new optional option of the fc_find_average_upper_limit. Add "try and except" for calculation of average upper limit and give user useful instructions on how to fix the problem.

Fixed an error in the documentation and removed the now unnecessary selection of x-bins.

Dirk Lennarz added 2 commits Dec 8, 2016
… (the value for low probability is a new option of the fc_find_average_upper_limit). Add try and except for calculation of average upper limit and give user useful instructions on how to fix the problem.
@dlennarz dlennarz changed the title Fc change Improvments to Feldman Cousins code Dec 8, 2016
@cdeil cdeil self-assigned this Dec 9, 2016
@cdeil cdeil added this to the 0.6 milestone Dec 9, 2016
Copy link
Member

@cdeil cdeil left a comment

Thanks!

I've left two minor inline comments.

Otherwise, this is ready to go.
(I can't comment on the method / stats aspect, there I just trust it makes sense, and I'm OK to put it in without adding an extra test for this.)

@@ -380,6 +381,9 @@ def fc_find_average_upper_limit(x_bins, matrix, upper_limit, mu_bins):
Feldman Cousins upper limit x-coordinates
mu_bins : array-like
The bins used in mue direction.
prob_limit : double

This comment has been minimized.

@cdeil

cdeil Dec 9, 2016
Member

double -> float (no such thing as a type double in Python)

This comment has been minimized.

@dlennarz

dlennarz Dec 10, 2016
Author Contributor

Funny that you notice this now, I have used double everywhere in the documentation so far ;)

Changed.

limit = fc_find_limit(x_bins[i], upper_limit, mu_bins)
except:
print("Warning: Calculation of average limit incomplete!")
print("Add more bins in mue direction or decrease prob_limit.")

This comment has been minimized.

@cdeil

cdeil Dec 9, 2016
Member

mue -> mu.

You should never print from library code, because then users (e.g. from pipelines) don't have easy control over the output. You can either emit a Warning or call log.warning.
Here's an explanation of how we call log.warning from Gammapy:
http://docs.gammapy.org/en/latest/development/howto.html#generating-log-messages

This comment has been minimized.

@dlennarz

dlennarz Dec 10, 2016
Author Contributor

Changed.

@dlennarz
Copy link
Contributor Author

@dlennarz dlennarz commented Dec 10, 2016

All requested changed implemented. Should be ready to merge.

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 12, 2016

@dlennarz - Thank you!

@cdeil cdeil merged commit ac2d0c9 into gammapy:master Dec 12, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil cdeil changed the title Improvments to Feldman Cousins code Improve Feldman Cousins code Dec 12, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Dec 12, 2016

There were some more parameters declared as double instead of float in Gammapy.
:-)
Fixed in 9a8c953

@dlennarz dlennarz deleted the dlennarz:fc_change branch Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants