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 convolution code and tests #1697

Merged
merged 3 commits into from Aug 12, 2018

Conversation

2 participants
@adonath
Copy link
Member

adonath commented Aug 12, 2018

This PR is a follow up to #1695. It implements the changes suggested by @cdeil:

  • Move the convolution unit tests to ´gammapy.maps´
  • Reintroduce the pixel scale check against the convolution kernel
  • Use fft convolution in the computation of Li & Ma images as well

Changing to fft convolution in ´compute_lima_image´ did not change the test result (except at the boundary, but I changed the boundary handling on purpose, as the boundary values where previously all set to np.nan), so I presume "ringing" does not seem to be an issue here.

@adonath adonath self-assigned this Aug 12, 2018

@adonath adonath added the cleanup label Aug 12, 2018

@adonath adonath added this to To do in Map analysis via automation Aug 12, 2018

@adonath adonath added this to the 0.8 milestone Aug 12, 2018

@cdeil

cdeil approved these changes Aug 12, 2018

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Aug 12, 2018

@adonath - Thanks!

@cdeil cdeil merged commit fe9d288 into gammapy:master Aug 12, 2018

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

Map analysis automation moved this from To do to Done Aug 12, 2018

@adonath adonath deleted the adonath:follow_up_to_#1695 branch Aug 12, 2018

@cdeil cdeil changed the title Follow up PR to #1695 Improve convolution code and tests Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.