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 ring background estimation #713

Merged
merged 12 commits into from Sep 21, 2016

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Sep 20, 2016

This PR includes the following changes:

  • Introduce RingBackgroundEstimator, superseeding the old RingBgMaker class
  • Implement SkyImage.convolve, SkyImage.__mul__ and SkyImage.__truediv__ methods
  • Removes the old ring_correlate function, which is now RingBackgroundEstimator.ring_convolve
  • Fix for #711

ToDo:

  • Add more tests
  • Add examples how to use RingBackgroundEstimator

@adonath adonath added this to the 0.5 milestone Sep 20, 2016

@cdeil

@adonath - I've left inline comments with my thoughts.

If you disagree with some of them, maybe come by and we discuss instead of via GH?

Show outdated Hide outdated gammapy/image/core.py
Show outdated Hide outdated gammapy/image/core.py
Show outdated Hide outdated gammapy/image/core.py
r = RingBgMaker(10, 13, 1)
r.correlate_maps(images)
class TestRingBackgroundEstimator:

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016

Member

The current test uses real data and has expected values where I don't know if they are correct or not.

Instead I would suggest you put constant test images, where the expected answer can be easily figured out.
It's also faster (no disk read, small image size).
And in addition to the test on a pixel in the center, add one for a pixel at the edge where the boundary handling matters, to make sure this is reasonable by default / doesn't change by accident in the future.

@cdeil

cdeil Sep 21, 2016

Member

The current test uses real data and has expected values where I don't know if they are correct or not.

Instead I would suggest you put constant test images, where the expected answer can be easily figured out.
It's also faster (no disk read, small image size).
And in addition to the test on a pixel in the center, add one for a pixel at the edge where the boundary handling matters, to make sure this is reasonable by default / doesn't change by accident in the future.

"""Ring background method for cartesian coordinates.
class RingBackgroundEstimator(object):
"""
Ring background method for cartesian coordinates.

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016

Member

Add a link to the gammapy.detect.KernelBackgroundEstimator in the docstring.

@cdeil

cdeil Sep 21, 2016

Member

Add a link to the gammapy.detect.KernelBackgroundEstimator in the docstring.

@cdeil cdeil self-assigned this Sep 21, 2016

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Sep 21, 2016

Member

Travis build passed...OK to merge, @cdeil?

Member

adonath commented Sep 21, 2016

Travis build passed...OK to merge, @cdeil?

@adonath adonath merged commit 905745f into gammapy:master Sep 21, 2016

1 of 2 checks passed

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

@adonath adonath referenced this pull request Sep 22, 2016

Closed

Add SkyImage.smooth method #694

@cdeil cdeil changed the title from Rework ring background to Improve ring background estimation Nov 18, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 18, 2016

Member

@adonath - I'm filling the 0.5 changelog. One thing I noticed is that you always put "rework" and otherwise I've always put "improve".

I think it's good if all changelog entries are uniform, usually one of "Add", "Fix" or "Improve".
OK?

Member

cdeil commented Nov 18, 2016

@adonath - I'm filling the 0.5 changelog. One thing I noticed is that you always put "rework" and otherwise I've always put "improve".

I think it's good if all changelog entries are uniform, usually one of "Add", "Fix" or "Improve".
OK?

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 18, 2016

Member

@cdeil Fine by me.

Member

adonath commented Nov 18, 2016

@cdeil Fine by me.

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