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

Add skyimage smooth method #774

Merged
merged 6 commits into from Nov 16, 2016

Conversation

@adonath
Copy link
Member

@adonath adonath commented Nov 14, 2016

This PR addresses issue #694:

  • Finish implementation of SkyImage.smooth() using parameter definitions of ds9
  • Remove disk_correlate and related functions from gammapy/image/utils
  • Add minimal test for all smoothing options
@adonath adonath added this to the 0.5 milestone Nov 14, 2016
@adonath adonath self-assigned this Nov 14, 2016
Copy link
Member

@cdeil cdeil left a comment

I've left some inline comments. Probably establishing kernel normalisation via the test is the most important one.

PS: one thing I don't like about Astropy kernels is that they sometimes emit this warning:

WARNING:gammapy.detect.lima:Using weighted kernels can lead to biased results.
WARNING: The kernel normalization factor is exceptionally large, > 100. [astropy.convolution.core]
WARNING:astropy:The kernel normalization factor is exceptionally large, > 100.

This is currently present in our docs build, and should be silenced. Same here for SkyImage.smooth, we never should emit that warning just because someone has a large kernel. So either not use Astropy kernel smooth, or catch and ignore the warning.

@@ -1,4 +1,4 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
# Licensed under a 3-clause BSD style license - see LICENSE.rst

This comment has been minimized.

@cdeil

cdeil Nov 15, 2016
Member

type: dedent this line again.

This comment has been minimized.

@adonath

adonath Nov 15, 2016
Author Member

Fixed.

desired = self.image.data.sum()
smoothed = self.image.smooth(kernel, 0.2 * u.deg)
actual = smoothed.data.sum()
assert_allclose(actual, desired)

This comment has been minimized.

@cdeil

cdeil Nov 15, 2016
Member

Is this a useful test?
The test image is all zero, no?
So smoothing with anything will pass the test.

A better test would depend on:

  • Kernel normalisation
  • Convolution behaviour at the edges
    and establish the current behaviour via asserts.

This comment has been minimized.

@adonath

adonath Nov 15, 2016
Author Member

Thanks, I missed, that the image is in fact empty. Fixed.

# use geometric mean if x an y pixel scale differ
radius = gmean((radius / self.wcs_pixel_scale()).value)

if kernel in ['box', 'disk']:

This comment has been minimized.

@cdeil

cdeil Nov 15, 2016
Member

I would put the width= lines below, instead of a separate if/else block here, even if it means duplicating one simple line for box and disk. But that's a matter of taste of course, if you prefer to leave it here, OK!

This comment has been minimized.

@adonath

adonath Nov 15, 2016
Author Member

Fixed.

@adonath adonath force-pushed the adonath:implement_skyimage_smooth_method branch from 34934b2 to 66949e3 Nov 15, 2016
@adonath adonath merged commit b83ad8c into gammapy:master Nov 16, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adonath adonath deleted the adonath:implement_skyimage_smooth_method branch Nov 16, 2016
@cdeil cdeil changed the title Implement skyimage smooth method Add skyimage smooth method Nov 18, 2016
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

2 participants