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 find_peaks for images #1759

Merged
merged 2 commits into from Sep 4, 2018
Merged

Add find_peaks for images #1759

merged 2 commits into from Sep 4, 2018

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 4, 2018

We have two tutorial notebooks that show how to use photutils.detection.find_peaks:

https://github.com/gammapy/gammapy-extra/search?q=find_peaks&unscoped_q=find_peaks

There was a change in photutils 0.5:
astropy/photutils#656

And I also put a compatibility code in
https://github.com/gammapy/gammapy-extra/blob/66ee6554e0636fc6fddfa176272d79d713c27a14/notebooks/cta_data_analysis.ipynb

# photutils changed the way to report the position
# the following code makes it work in both old and new versions
try:
    # photutils 0.5 or newer
    source_pos = sources['skycoord_peak']
except KeyError:
    # photutils 0.4 or older
    source_pos = SkyCoord(sources['icrs_ra_peak'], sources['icrs_dec_peak'])
source_pos

I would suggest to put some find_peaks in gammapy.image and avoid the photutils dependency. Either copy the latest version from there to gammapy.extern, or just copy the parts that call scipy.ndimage from there or scikit-image, i.e. put a simple 10 line version of a peak finder. We don't need a lot of bells and whistles and sub-pixel accuracy, people will anyways just use that to find the peak roughly and then fit sources individually.

For the notebooks: maybe we could / should avoid the duplication, and just show how to do it once and add a link to the other?

A very commmon question is how to plot region circles. I think this example would be the best place to show that, making a regions circle object and plotting it instead of calling scatter. So that's another small change to make when touching the notebooks.

@adonath - Thoughts on what exactly to do? Do you have time in the next days of should I?

@cdeil cdeil added bug cleanup labels Aug 31, 2018
@cdeil cdeil added this to the 0.8 milestone Aug 31, 2018
@adonath
Copy link
Member

@adonath adonath commented Aug 31, 2018

In this case I think it's also justified to drop the support for the older version. Right now it's the only place in gammapy or gammapy-extra where we use photutils. One could update the https://github.com/gammapy/gammapy-extra/blob/master/environment.yml and require photutils >= 0.5.

Users can always easily update their existing environments using:

conda env update -f environment.yml

Maybe we should document this somewhere...

@cdeil cdeil self-assigned this Sep 3, 2018
@cdeil cdeil added this to To do in gammapy.maps via automation Sep 3, 2018
@cdeil cdeil force-pushed the cdeil:peak branch from 3e284a1 to 6be39e2 Sep 4, 2018
@cdeil
Copy link
Member Author

@cdeil cdeil commented Sep 4, 2018

@adonath - I did decide to add gammapy.image.find_peaks. The core algorithm of using a max filter is the same in photutils and scikit-image peak_local_max and just 5 lines of code, so IMO not worth an extra dependency. I did expose the min_distance argument as in peak_local_max, because I think it's pretty intuitive. Maybe we should add another assert that this is an int if non-int values could lead to shifts (I didn't test that).

CI is broken at the moment (known issue: astropy/ci-helpers#308), so I'll merge this now and update the notebooks.

But @adonath - of course your review here is still welcome. If you want to improve something, feel free to do it directly via a follow-up commit in master.

@cdeil cdeil force-pushed the cdeil:peak branch from 6be39e2 to a6f2059 Sep 4, 2018
@cdeil
Copy link
Member Author

@cdeil cdeil commented Sep 4, 2018

I've removed photutils as an optional dependency in a6f2059

@cdeil cdeil merged commit 74c1cc4 into gammapy:master Sep 4, 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 Sep 4, 2018
@cdeil
Copy link
Member Author

@cdeil cdeil commented Sep 4, 2018

Oups, I guess find_peaks belongs in gammapy.detect. I'll move it there now via a commit in master.

@cdeil
Copy link
Member Author

@cdeil cdeil commented Sep 4, 2018

I updated the two notebooks using find_peaks. This task is done.

@cdeil cdeil changed the title Add find_peaks in gammapy.image Add find_peaks for images Sep 9, 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