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

Refactor ring background maker #2520

Merged
merged 11 commits into from Nov 9, 2019

Conversation

@luca-giunti
Copy link
Contributor

luca-giunti commented Nov 6, 2019

In this PR, I refactor the RingBackgroundEstimator into RingBackgroundMaker and AdaptiveRingBackgroundEstimator into AdaptiveRingBackgroundMaker. For the moment, those are still two independent makers, but could eventually be merged into a single maker later on.

I compared the performances of the new "maker" pattern to the old scheme. You can see this comparison in two notebooks:

Using 4 runs, the computation time for the non-adaptive ring algorithm drops from 5.46 s ± 469 ms to 2.61 s ± 125 ms, and for the adaptive ring from 7.52 s ± 622 ms to 3.08 s ± 334 ms.

Also,the results look very similar. However, as you can see in the notebooks, there is a small shift in alpha: taking a random bin, I find a shift from 0.00077920605 to 0.000740094. I think this might be related to the fact that in v0.13 we were stacking the off_exposure using a simple fill_by_coord() (see here), i.e. no weighting by the obs time or off counts was performed... Is that possible?

@luca-giunti luca-giunti requested a review from adonath Nov 6, 2019
@cdeil cdeil added the cleanup label Nov 6, 2019
@cdeil cdeil added this to In progress in gammapy.cube via automation Nov 6, 2019
@cdeil cdeil added this to the 0.15 milestone Nov 6, 2019
Copy link
Member

adonath left a comment

Thanks @luca-giunti! I've left a few inline comments.

See Also
--------
RingBackgroundEstimator, gammapy.detect.KernelBackgroundEstimator
RingBackgroundMaker, gammapy.detect.KernelBackgroundMaker

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

You accidentally rename the KernelBackgroundEstimator...

@@ -1011,7 +1011,10 @@ def parameters(self):
@property
def alpha(self):
"""Exposure ratio between signal and background regions"""
return self.acceptance / self.acceptance_off
tmp = self.acceptance / self.acceptance_off
mask = np.where(np.isnan(tmp.data))

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

Are you sure this statement has an effect? Because I think Numpy returns np.inf for division by zero:

import numpy as np
np.isnan(np.array([1.]) / np.array([0.]))

This comment has been minimized.

Copy link
@luca-giunti

luca-giunti Nov 8, 2019

Author Contributor

Yes, it has an effect. If I remove these two lines, the acceptance_off of the stacked map (after the ring background estimation) looks like thistest

This comment has been minimized.

Copy link
@adonath

adonath Nov 8, 2019

Member

Sorry, I didn't notice, that you set np.nan explicitly as a placeholder here: https://github.com/gammapy/gammapy/pull/2520/files#diff-5c957f91195d9d25604ec77a67ba3ceeR323
OK, to keep for now, but I think in general we should try to simplify the data masking as much as possible, basically leaving the data untouched and take into account and modify the safe_mask instead where needed (can be done in a follow up PR...).

----------
dataset : `~gammapy.cube.fit.MapDataset`
Input map dataset.
exclusion : `~gammapy.maps.WcsNDMap`

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

In the ReflectedRegionsBackgroundMaker the exclusion_mask is taken on __init__. I think for consistency it would be good to also move it to RingBackgroundMaker.__init__(). Alternatively we could also attach it to the dataset, but this we should discuss as separate issue.

This comment has been minimized.

Copy link
@luca-giunti

luca-giunti Nov 8, 2019

Author Contributor

Done. In the new implementation, I had to add a fill_by_coord() to reproject internally the exclusion_mask to the cutout geom. With 4 runs, I checked that this increases the computation time of ~4%, which is not too bad I guess

This comment has been minimized.

Copy link
@adonath

adonath Nov 8, 2019

Member

Yes, this is completely acceptable.


class RingBackgroundEstimator:
fft_noise_threshold = 1e-6

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

This hard-coded threshold seem problematic to me. I guess in general the acceptance_off is zero, where the ring completely falls into an exclusion region, no? What if we ring-convolve the exclusion_mask as well and derive the off mask from this? I guess we can also use scripy.ndimage.morphology. Here is a minimal example:

from astropy.convolution import Ring2DKernel
from scipy.ndimage.morphology import binary_erosion, binary_dilation
from gammapy.maps import Map
from regions import CircleSkyRegion
from astropy import units as u
from astropy.coordinates import SkyCoord

binsz= 0.02
center = SkyCoord("0 deg", "0 deg")
circle = CircleSkyRegion(center=center, radius=0.4 *u.deg)

exclusion_mask = Map.create(width=2, binsz=binsz)
exclusion_mask.data = exclusion_mask.geom.region_mask([circle], inside=True)

ring = Ring2DKernel(10, width=5)
ring.normalize("peak")

data = binary_erosion(exclusion_mask.data, structure=ring.array)
mask = exclusion_mask.copy(data=data)
acceptance_off.data[not_has_acceptance] = 0
acceptance.data[not_has_acceptance] = 0

mask_safe = dataset.counts.copy(

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

I think we should definitely keep and use the mask_safe, probably instead of modifying the data, as you did above. My naive proposal would be, that the mask_safe should be taken into account when convolving the acceptance (instead of relying on it to be zero) and then the mask_safe should be modified to exclude regions with no acceptance_off (computed as I proposed above).

I have to think about it once more, to get a clearer picture of how to handle the mask_safe here, but I think in general we should leave the data untouched (not setting anything to zero or NaN) but keep track of the information in the mask_safe instead. When the dataset is stacked the data is modified anyway (set to zero). And this gives a single, well defined place, where the data is touched.

As I said: I'll think about it once more and make a proposal. It should not stop us form merging this PR.

mask_fit=dataset.mask_fit,
psf=dataset.psf,
edisp=dataset.edisp,
background_model=None,

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

No need to redefine the default value.

edisp=dataset.edisp,
background_model=None,
name=dataset.name,
evaluation_mode="local",

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

Again no need to redefine the default.

def test_adaptive_ring_bkg_maker(map_dataset_maker, observations, exclusion_region):
datasets = {}

for method in ["fixed_width", "fixed_r_in"]:

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

The way we typically test different parametrisation is by using pytest.mark.parametrise() see e.g. https://github.com/gammapy/gammapy/blob/master/gammapy/cube/tests/test_make.py#L35
This has the advantage, that parametrised test fail independently. However in the current implementation it's not too bad, because you have separate asserts. On the other hand pytest.mark.parametrise() would reduce the code duplication even more. So I'd prefer to change it...

def observations():
"""Example observation list for testing."""
datastore = DataStore.from_file(
"$GAMMAPY_DATA/hess-dl3-dr1/hess-dl3-dr3-with-background.fits.gz"

This comment has been minimized.

Copy link
@adonath

adonath Nov 7, 2019

Member

One more comment: this currently fails on Travis, because we removed the old background models. Just change this to DataStore.from_dir("$GAMMAPY_DATA/hess-dl3-dr1") and it should work again. Locally you have to update you gammapy data folder by deleting it and running make dataset-download.

@luca-giunti

This comment has been minimized.

Copy link
Contributor Author

luca-giunti commented Nov 8, 2019

Thanks @adonath for the review! I implemented what you requested, with the exception of the following comments:

  • #2520 (comment): As you said, we need a proposal for the mask_safe handling
  • #2520 (comment): I completely agree that the hard-coded value is bad. As you suggested, I tried to implement a solution based on scripy.ndimage.morphology but I can't get it to work...maybe, if you have it clear in mind could you add this yourself before merging?
@adonath adonath force-pushed the luca-giunti:Refactor-RingBackgroundMaker branch from 6a56f7e to c6d6522 Nov 9, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #2520 into master will decrease coverage by 0.27%.
The diff coverage is 94.84%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2520      +/-   ##
=========================================
- Coverage   91.08%   90.8%   -0.28%     
=========================================
  Files         147     147              
  Lines       16871   16886      +15     
=========================================
- Hits        15367   15334      -33     
- Misses       1504    1552      +48
Impacted Files Coverage Δ
gammapy/cube/fit.py 89.41% <100%> (+0.03%) ⬆️
gammapy/cube/ring.py 94.61% <94.79%> (-1.97%) ⬇️
gammapy/cube/make.py 73.15% <0%> (-23.69%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 652f1d2...ecd7274. Read the comment docs.

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 9, 2019

Thanks a lot @luca-giunti! I added a few clean-up commits addressing the following things:

  • I removed the .parameters attribute from the makers and exposed the parameters directly on the class (mainly for consistency with the other Maker classes)
  • I simplified the exclusion mask reprojection a bit to avoid the .fill_by_coord() call
  • I also simplified the data masking to not modify the data, but just the mask_safe, the mask safe is applied consistently on .stack() setting the data off counts_off and acceptance_off to zero.
  • I already adapted the image analysis tutorial, because it was failing on Travis-CI

I'll wait for Travis to pass and the go ahead and merge.

@adonath adonath force-pushed the luca-giunti:Refactor-RingBackgroundMaker branch from c6d6522 to ecd7274 Nov 9, 2019
@adonath adonath merged commit 1b9e0d0 into gammapy:master Nov 9, 2019
10 of 11 checks passed
10 of 11 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
Scrutinizer Analysis: 1 new issues, 17 updated code elements – Tests: passed
Details
codecov/patch 94.84% of diff hit (target 91.08%)
Details
codecov/project Absolute coverage decreased by -0.27% but relative coverage increased by +3.76% compared to 652f1d2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191109.4 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
gammapy.cube automation moved this from In progress to Done Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.cube
  
Done
3 participants
You can’t perform that action at this time.