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

Fix TS maps low stats handling #635

Merged
merged 3 commits into from Jul 19, 2016

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Jul 18, 2016

This PR fixes the handling of pixels with low stats for TS map computation. It further includes some clean up and restructuring of test_image_ts.py

b_min = c_min / s_model - sn_min
b_max = s_counts / s_model - sn_min
return b_min / FLUX_FACTOR, b_max / FLUX_FACTOR
return b_min / FLUX_FACTOR, b_max / FLUX_FACTOR, -sn_min_total / FLUX_FACTOR

This comment has been minimized.

@cdeil

cdeil Jul 18, 2016

Member

Can you change to return a NamedTuple or dict, please?

Calling code is always easier to read if functions that return multiple things give names to those things instead of using result[0], result[1], result[2]`.

There's a few more functions in Gammapy that return tuples, I suggest we change all of them to the more readable alternative.

@cdeil

cdeil Jul 18, 2016

Member

Can you change to return a NamedTuple or dict, please?

Calling code is always easier to read if functions that return multiple things give names to those things instead of using result[0], result[1], result[2]`.

There's a few more functions in Gammapy that return tuples, I suggest we change all of them to the more readable alternative.

This comment has been minimized.

@adonath

adonath Jul 18, 2016

Member

This function is hidden, very low-level and not meant to be used outside the _ts_value() functions (and I can hardly imagine any other use case...). So I kept the simpler version with returning a tuple. For developers the returned values should be self-explaining by their name.

@adonath

adonath Jul 18, 2016

Member

This function is hidden, very low-level and not meant to be used outside the _ts_value() functions (and I can hardly imagine any other use case...). So I kept the simpler version with returning a tuple. For developers the returned values should be self-explaining by their name.

@cdeil cdeil added bug cleanup labels Jul 18, 2016

@cdeil cdeil added this to the 0.5 milestone Jul 18, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 18, 2016

Member

I had a quick look and left two superficial inline comments. Otherwise, 👍 , thanks!

Member

cdeil commented Jul 18, 2016

I had a quick look and left two superficial inline comments. Otherwise, 👍 , thanks!

@cdeil cdeil merged commit 51bdf80 into gammapy:master Jul 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 19, 2016

Member

Thanks!

Member

cdeil commented Jul 19, 2016

Thanks!

@cdeil cdeil changed the title from Fix ts maps low stats handling to Fix TS maps low stats handling Aug 23, 2016

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