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

Restructure TS map computation #215

Merged
merged 1 commit into from Dec 11, 2014

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Dec 3, 2014

As close to final as it could get today.

Show outdated Hide outdated gammapy/detect/__init__.py Outdated
Show outdated Hide outdated gammapy/detect/utils.py Outdated
Show outdated Hide outdated scripts/gammapy-residual-ts-image Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 3, 2014

Member

Great ... I'll have a closer look when travis-ci passes. ;-)

Member

cdeil commented Dec 3, 2014

Great ... I'll have a closer look when travis-ci passes. ;-)

Show outdated Hide outdated astropy_helpers Outdated
@@ -0,0 +1,511 @@
from __future__ import division, print_function, absolute_import

This comment has been minimized.

@cdeil

cdeil Dec 4, 2014

Member

Please leave a comment in this file or the README in extern where this code came from and if you modified it, briefly what you changed.

@cdeil

cdeil Dec 4, 2014

Member

Please leave a comment in this file or the README in extern where this code came from and if you modified it, briefly what you changed.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 4, 2014

Member

Why did you change test_statistic.py to teststatistics.py?
Usually in Astropy and Gammapy we use singular and separate words by underscores, i.e. if you don't mind much, could you please rename back to test_statistic.py?

Also you have test_teststatistic.py, i.e. without the plural s.
Simply always having test_ prepended seems simpler to me.

Member

cdeil commented Dec 4, 2014

Why did you change test_statistic.py to teststatistics.py?
Usually in Astropy and Gammapy we use singular and separate words by underscores, i.e. if you don't mind much, could you please rename back to test_statistic.py?

Also you have test_teststatistic.py, i.e. without the plural s.
Simply always having test_ prepended seems simpler to me.

Show outdated Hide outdated gammapy/detect/teststatistics.py Outdated
Show outdated Hide outdated gammapy/detect/teststatistics.py Outdated
Show outdated Hide outdated gammapy/detect/teststatistics.py Outdated
Show outdated Hide outdated gammapy/detect/teststatistics.py Outdated
Show outdated Hide outdated gammapy/irf/psf_core.py Outdated
Show outdated Hide outdated scripts/gammapy-residual-ts-image Outdated
Show outdated Hide outdated scripts/gammapy-residual-ts-image Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 4, 2014

Member

I'm getting this test error for test_compute_ts_map with Python 3.4 on Mac:
https://gist.github.com/cdeil/45051376b76f9cf0c9f1

Any idea what's going on or how to fix it?

Member

cdeil commented Dec 4, 2014

I'm getting this test error for test_compute_ts_map with Python 3.4 on Mac:
https://gist.github.com/cdeil/45051376b76f9cf0c9f1

Any idea what's going on or how to fix it?

Show outdated Hide outdated gammapy/detect/test_statistics.py Outdated
Show outdated Hide outdated gammapy/detect/test_statistics.py Outdated
Show outdated Hide outdated scripts/gammapy-residual-ts-image Outdated
Show outdated Hide outdated scripts/gammapy-residual-ts-image Outdated
Show outdated Hide outdated scripts/gammapy-residual-ts-image Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 4, 2014

Member

I'm now getting this error:

gammapy/detect/tests/test_test_statistic.py:6: in <module>
>   from ...detect import ts_image, TSMapCalculator
E   ImportError: cannot import name 'ts_image'
Member

cdeil commented Dec 4, 2014

I'm now getting this error:

gammapy/detect/tests/test_test_statistic.py:6: in <module>
>   from ...detect import ts_image, TSMapCalculator
E   ImportError: cannot import name 'ts_image'
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 4, 2014

Member

The test is not running on travis-ci because scipy is not installed.
I'm fixing .travis.yml in a separate PR now and will then restart the tests here.

Member

cdeil commented Dec 4, 2014

The test is not running on travis-ci because scipy is not installed.
I'm fixing .travis.yml in a separate PR now and will then restart the tests here.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 4, 2014

Member

I've restarted the travis-ci build ... scipy is now installed and the TS map test should run.

Please also address the minor comments I made above before merging this (you can see them all on the Github pull request page as inline comments).

Member

cdeil commented Dec 4, 2014

I've restarted the travis-ci build ... scipy is now installed and the TS map test should run.

Please also address the minor comments I made above before merging this (you can see them all on the Github pull request page as inline comments).

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Dec 9, 2014

Member

I've adressed most of the comments and Travis builds have passed. Ready to merge?

Member

adonath commented Dec 9, 2014

I've adressed most of the comments and Travis builds have passed. Ready to merge?

Show outdated Hide outdated gammapy/image/plotting.py Outdated
Show outdated Hide outdated gammapy/image/plotting.py Outdated
Show outdated Hide outdated gammapy/image/tests/test_measure.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
from skimage.measure import block_reduce
if not np.log2(factor).is_integer():
raise ValueError('Downsampling factor must be power of 2.')
factor = int(factor)

This comment has been minimized.

@cdeil

cdeil Dec 9, 2014

Member

Add empty line.

@cdeil

cdeil Dec 9, 2014

Member

Add empty line.

Show outdated Hide outdated gammapy/image/utils.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
@@ -324,7 +324,7 @@ def _get_psf(self, index):
return psf
def multi_gauss_psf_kernel(psf_parameters, **kwargs):
def multi_gauss_psf_kernel(psf_parameters, BINSZ=0.02, NEW_BINSZ=0.02, **kwargs):

This comment has been minimized.

@cdeil

cdeil Dec 9, 2014

Member

Remove this default of 0.02 deg ... most users will need something else and forcing them to explicitly give this parameter is better.

@cdeil

cdeil Dec 9, 2014

Member

Remove this default of 0.02 deg ... most users will need something else and forcing them to explicitly give this parameter is better.

Show outdated Hide outdated gammapy/morphology/tests/test_gauss.py Outdated
Show outdated Hide outdated scripts/gammapy-ts-image Outdated
for name, order in zip(['ts', 'amplitude', 'niter'], [3, 3, 0]):
if name == 'ts':
# We don't allow negative TS values and clip
ts_results[name] = np.clip(ts_results[name], 0, np.inf)

This comment has been minimized.

@cdeil

cdeil Dec 9, 2014

Member

Add empty line after this one.

@cdeil

cdeil Dec 9, 2014

Member

Add empty line after this one.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 10, 2014

Member

41d2e90 in master should fix the unrelated apt-get error on travis-ci.

Member

cdeil commented Dec 10, 2014

41d2e90 in master should fix the unrelated apt-get error on travis-ci.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 10, 2014

Coverage Status

Coverage remained the same when pulling c4acefa on adonath:ts_map_calculator_parallel into 41d2e90 on gammapy:master.

coveralls commented Dec 10, 2014

Coverage Status

Coverage remained the same when pulling c4acefa on adonath:ts_map_calculator_parallel into 41d2e90 on gammapy:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 10, 2014

Coverage Status

Coverage remained the same when pulling 66f2869 on adonath:ts_map_calculator_parallel into 41d2e90 on gammapy:master.

coveralls commented Dec 10, 2014

Coverage Status

Coverage remained the same when pulling 66f2869 on adonath:ts_map_calculator_parallel into 41d2e90 on gammapy:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 10, 2014

Coverage Status

Coverage remained the same when pulling 66f2869 on adonath:ts_map_calculator_parallel into 41d2e90 on gammapy:master.

coveralls commented Dec 10, 2014

Coverage Status

Coverage remained the same when pulling 66f2869 on adonath:ts_map_calculator_parallel into 41d2e90 on gammapy:master.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Dec 10, 2014

Member

@adonath You want to hit the merge button?
Or was there something else you wanted to change here?

Member

cdeil commented Dec 10, 2014

@adonath You want to hit the merge button?
Or was there something else you wanted to change here?

adonath added a commit that referenced this pull request Dec 11, 2014

@adonath adonath merged commit 3e0f978 into gammapy:master Dec 11, 2014

1 of 2 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci The Travis CI build passed
Details

@cdeil cdeil referenced this pull request Feb 5, 2015

Closed

Further TS map improvements #223

6 of 10 tasks complete

@cdeil cdeil changed the title from Restructured TS map computation to Restructure TS map computation Apr 8, 2015

@cdeil cdeil added the feature label Apr 8, 2015

@cdeil cdeil added this to the 0.2 milestone Apr 8, 2015

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