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

Merged
merged 5 commits into from Sep 23, 2016

Conversation

@adonath
Copy link
Member

@adonath adonath commented Sep 21, 2016

This PR adds a new AdaptiveRingBackgroundEstimator class, that implements the adaptive ring background method.

@adonath adonath added this to the 0.5 milestone Sep 21, 2016
Copy link
Member

@cdeil cdeil left a comment

I've left a bunch of inline comments.

Overall the code looks clean and nice, most comments are suggestions for consideration and it'd also be OK to leave as-is if you prefer.

I didn't review the core algorithm, it would be nice to have a few more asserts that show that the ring enlarges as it should by the algorithm definition (by looking at ring parameters and total pixels as well as pixels outside exclusion in the ring, for two positions (a. where the ring doesn't adapt, b. where it does adaptively enlarge).

size of the ring to achieve a minimum on / off exposure ratio (alpha) in regions
where the area to estimate the background from is limited.
Here is an illustration of the method:

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

Maybe put high-level docs like method illustrations in docs instead of having it in the API docs section?
(as you like, I'm not sure it's better ...)

Width of the ring.
stepsize : `~astropy.units.Quantity` (0.02 deg)
Stepsize used for increasing the radius.
threshold : float (0.1)

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

Is this a threshold on alpha? Maybe put "alpha" in the parameter name or one-line description?

Threshold above which the adaptive ring takes action.
theta : `~astropy.units.Quantity` (0.22 deg)
Integration radius used for alpha computation.
method : {'const. width', 'const. r_in'}

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

Maybe use something that's a bit easier to type correctly (no .) and can be used e.g. in filenames more easily, e.g. fix_width and fix_r_in ?

def __init__(self, r_in, r_out_max, width, stepsize=None,
threshold=0.1, theta=None, method='const. width'):

# default values

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

I've thought about this a lot and had discussions with others.
The plan is to use Quantity objects as default kwargs, even if they are mutable.
So I'd be +1 to put the default values in the signature and remove them from here and the docstring, to make maintenance a little easier (only have the default in one place, not two).

This comment has been minimized.

@adonath

adonath Sep 22, 2016
Author Member

Done

if method not in ['const. width', 'const. r_in']:
raise ValueError("Not a valid adaptive ring method.")

self.parameters = dict(r_in=r_in, r_out_max=r_out_max, width=width,

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

I've started using OrderedDict everywhere. There's no cost, and it has some advantages, e.g. print output is or storing it in JSON or a header is always the same order.

This comment has been minimized.

@adonath

adonath Sep 22, 2016
Author Member

Done

"""
p = self.parameters

scale = image.wcs_pixel_scale()[0]

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

Maybe pass images to __init__?
Then the pix scale and number of kernels and their parameters would be known and could e.g. be printed in the class __str__.

Really objects like this Estimator are supposed to be instantiated, run and results read off.
Then if you need to run with different parameters or a new set of images, you just make a new one, no big deal.

kernels: list of `~astropy.convolution.Kernel`
List of convolution kernels.
parallel : bool
Whether to use multiprocessing.

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

Add Returns and maybe mention the shape of data and return array?

Also, what about the name?
The main thing this does is to convolve, no? So add "convolve" in?

def test_run(self):
result = self.ring.run(self.images)
assert_allclose(result['background'].data[50, 50], 1)
assert_allclose(result['alpha'].data[50, 50], 0.002638522427440632)

This comment has been minimized.

@cdeil

cdeil Sep 21, 2016
Member

Is there a way to debug if that alpha value is correct?
What matters is to be able to figure out which scale index was used for the pixel and how many pixels the ring kernel for that scale has.
Possible to access for experts / debugging after running the algorithm?

@@ -51,21 +267,25 @@ class RingBackgroundEstimator(object):
See Also
--------
KernelBackgroundEstimator
KernelBackgroundEstimator, AdaptiveRingBackgroundEstimator

This comment has been minimized.

@cdeil

cdeil Sep 22, 2016
Member

This link to KernelBackgroundEstimator doesn't work:
http://docs.gammapy.org/en/latest/api/gammapy.background.RingBackgroundEstimator.html

Maybe you just have to write it out in full?
gammapy.detect.KernelBackgroundEstimator ?

This comment has been minimized.

@adonath

adonath Sep 22, 2016
Author Member

Done

@adonath adonath force-pushed the adonath:implement_adaptive_ring branch from 1581310 to 3ea3286 Sep 22, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Sep 22, 2016

@adonath - I merged #721 just now. Looks like it didn't conflict with your ring PR branch here.

@adonath adonath force-pushed the adonath:implement_adaptive_ring branch from 3ea3286 to 3d7e0bd Sep 22, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Sep 22, 2016

@adonath - Can you please remove or check off the open task list at the top and remove WIP from the title?

Did you want to show adaptive_ring_bkg.png in the docs?
You're copying it over in docs/conf.py, but then you're not showing it.

@cdeil cdeil changed the title WIP: Implement adaptive ring background method Add adaptive ring background estimation Sep 23, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Sep 23, 2016

This PR is related to a fail I see in #705 : https://travis-ci.org/gammapy/gammapy/jobs/161942204#L2046

I'm merging this now and will continue a bit with this now.

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 23, 2016

@adonath - Thanks!

@cdeil cdeil merged commit 86ea359 into gammapy:master Sep 23, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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