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 WcsNDMap convolve method #1695

Merged
merged 6 commits into from Aug 10, 2018
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Aug 10, 2018

Image convolution is used in many places in Gammapy (gammapy.cube, gammapy.detect, gammapy.image). This PR introduces a WcsNDMap.convolve() method to unify the usage of image convolutions in Gammapy. So far different functions from astropy as well as scipy have beem used. To make a choice, which functions to use for WcsNDMap.convolve() I wrote a notebook comparing the performance of the different convolutions functions in astropy and scipy. The clear winners are scipy.signal.fftconvolve and scipy.ndimage.convolve. The notebook also shows, that for scipy.signal.fftconvolve the runtime only weakly depends on the kernel size. Choosing the fft convolution we might avoid the need for energy dependent kernel sizes.

@adonath adonath added this to the 0.8 milestone Aug 10, 2018
@adonath adonath self-assigned this Aug 10, 2018
@adonath adonath added this to In progress in gammapy.modeling via automation Aug 10, 2018
@adonath adonath requested a review from cdeil Aug 10, 2018
@adonath adonath added this to To do in gammapy.maps via automation Aug 10, 2018
@adonath adonath requested a review from registerrier Aug 10, 2018
@cdeil
cdeil approved these changes Aug 10, 2018
Copy link
Member

@cdeil cdeil left a comment

This looks great, thank you!

I'm merging this now.

Some things we might want to discuss / change next week:
Why is non-fft used for LiMa but not the others? Was there a reason not to switch to FFT for LiMA?
There was a check that the kernel pixel size roughly matches the map, that might be useful to add back.
I don't see any unit tests in the diff. weren't there some for the PSFKernel.apply that should now appear in the tests for Map.convolve?

@cdeil cdeil merged commit e9edc66 into gammapy:master Aug 10, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.maps automation moved this from To do to Done Aug 10, 2018
gammapy.modeling automation moved this from In progress to Done Aug 10, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Aug 10, 2018

This is what I get on my Macbook with 2,5 GHz Intel Core i7 and scipy from Anaconda:

perf

@cdeil cdeil changed the title Introduce WcsNDMap.convolve() method Add WcsNDMap convolve method Aug 10, 2018
@adonath
Copy link
Member Author

@adonath adonath commented Aug 11, 2018

@cdeil No there was no specific reason to use non-fft for Li&Ma images, except that I wanted to keep the tests unchanged for this PR. The unit tests are still in gammapy/cube/test/test_psf_kernel.py, I agree they should probably be copied over to gammapy.maps. I'll work on a follow up PR now and also reintroduce the pixel size check...

Unfortunately the results of the convolution comparison on your mac book is not visible, just an empty image. Does it give similar results?

@cdeil
Copy link
Member

@cdeil cdeil commented Aug 11, 2018

@adonath - Thanks!

Yes, the results were very similar, except that the crossing point where scipy.signal.fftconvolve is faster than scipy.ndimage.convolve was already a little sooner, at a kernel width of 6 or 7. So probably fftconvolve is the better default. Although that comes with the caveat that we haven't looked at it in some more detail yet, namely dependency of speed (and maybe also precision) on float type (32 and 64 bit) and on image size. Is fftconvolve well-behaved, i.e. similar for any image size? Or does it go in steps and e.g. 513 is much slower than 512?

@cdeil
Copy link
Member

@cdeil cdeil commented Aug 11, 2018

Does FFT convolve work well for kernels with hard edges, like a disk for the on region or a ring for the background? Or does it give ringing artefacts or anything like that?

cdeil added a commit that referenced this pull request Aug 12, 2018
Follow up PR to #1695
@adonath adonath deleted the adonath:unify_convolution_functions branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants