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

Fixed TS map boundary handling #332

Merged
merged 1 commit into from Aug 25, 2015

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Aug 19, 2015

This fixes the boundary behaviour of the TS map computation. I guess there's not much to review here. I will wait for the Travis-builds and then do the merge.

@adonath adonath force-pushed the adonath:fix_ts_boundary_handling branch from f60a432 to 90b2f23 Aug 19, 2015

@cdeil cdeil added the bug label Aug 19, 2015

@cdeil cdeil added this to the 0.4 milestone Aug 19, 2015

@@ -5,8 +5,9 @@
import json
import logging
log = logging.getLogger(__name__)
from ..utils.scripts import get_parser
from ..utils.scripts import get_parser, _configure_root_logger

This comment has been minimized.

@cdeil

cdeil Aug 19, 2015

Member

Can you use set_up_logging_from_args as described here or as used in this example here?
Or is there a reason this doesn't work properly here?

if mask_.any():
log.warn('There are pixels in the data, that have exposure, but zero '
'background, which can cause the ts computation to fail.'
'Setting exposure of this pixels to zero.')

This comment has been minimized.

@cdeil

cdeil Aug 19, 2015

Member

Add space in the string on the line before. And on this line: "this pixels" -> "these pixels".

# in some maps there are pixels, which have exposure, but zero
# background, which doesn't make sense and causes the TS computation
# to fail, this is a temporary fix
mask_ = np.logical_and(background == 0, exposure > 0)

This comment has been minimized.

@cdeil

cdeil Aug 19, 2015

Member

I think this is equivalent and a bit more readable?

bad_pixels = (background == 0) && (exposure > 0)

This comment has been minimized.

@adonath

adonath Aug 20, 2015

Member

Does the && operator even exists in Python? I don't think so... I just checked and it throws a SyntaxError in Python 2.7. In principle one could use the bitwise & operator because both arrays are boolean, but I think it's better to have the explicit np.logical_and, which corresponds to the element-wise Python and.

This comment has been minimized.

@cdeil

cdeil Aug 20, 2015

Member

I meant & where I typed &&.
If you think np.logical_and is better than &, sure, OK.

CHANGES.rst Outdated
Pull requests
+++++++++++++
- ...
- Fixed TS map boundary handling [#332] (Axel Donath)

This comment has been minimized.

@cdeil

cdeil Aug 19, 2015

Member

Thanks for adding a changelog entry. It's really minor and nitpicky, but for the future: please use active voice in GH issues / commits / changelog entries, i.e. "Fix" instead of "Fixed".

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 19, 2015

I left a few minor comments inline. Travis-ci passed, so this is almost ready to go.

@adonath adonath force-pushed the adonath:fix_ts_boundary_handling branch from 90b2f23 to 3cd22ea Aug 20, 2015

@adonath adonath force-pushed the adonath:fix_ts_boundary_handling branch from 3cd22ea to a751712 Aug 24, 2015

adonath added a commit that referenced this pull request Aug 25, 2015

@adonath adonath merged commit 6da03ed into gammapy:master Aug 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adonath adonath deleted the adonath:fix_ts_boundary_handling branch Nov 20, 2018

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