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

Implement additional methods for SafeMaskMaker #2604

Conversation

@luca-giunti
Copy link
Contributor

luca-giunti commented Nov 25, 2019

In this PR:

  • I implemented the SafeMaskMaker.make_mask_edisp_bias(...) method for MapDataset and MapDatasetOnOff, that was previously raising NotImplementedError
  • I implemented a new SafeMaskMaker.make_mask_bkg_peak(...) method, motivated by the HESS DL3 analysis validation paper (page 11, section 5.5.1)

As soon as this gets merged, I will resolve corresponding the TODO in the validation script. This will make the code simpler and shorter there.

@luca-giunti luca-giunti requested a review from adonath Nov 25, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #2604 into master will increase coverage by <.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2604      +/-   ##
=========================================
+ Coverage   91.49%   91.5%   +<.01%     
=========================================
  Files         140     140              
  Lines       15939   15959      +20     
=========================================
+ Hits        14584   14603      +19     
- Misses       1355    1356       +1
Impacted Files Coverage Δ
gammapy/cube/make.py 94.93% <91.66%> (-0.72%) ⬇️
gammapy/maps/wcsnd.py 96.28% <0%> (+0.3%) ⬆️

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 2ecaa77...115ab75. Read the comment docs.

Copy link
Member

adonath left a comment

Thanks @luca-giunti, I've left a few inline comments to address, before we can merge. Otherwise looks good to me.

raise NotImplementedError(
"'edisp-bias' method currently only supported for spectral datasets"
)
if self.position is None:

This comment has been minimized.

Copy link
@adonath

adonath Nov 25, 2019

Member

Maybe we compute the threshold at the center of the map as a default? However I'm not sure this is a safe assumption. Do you know @registerrier what is the typical quality of IRFs at offset=0 deg?


peak = background_spectrum.data.max()
idx = list(background_spectrum.data).index(peak)
e_min = background_spectrum.energy.center[idx]

This comment has been minimized.

Copy link
@adonath

adonath Nov 25, 2019

Member

Note the implementation differs from the description "upper edge of the energy bin with the highest predicted background rate". Please unify to whatever is described in the paper...

e_min = edisp.get_bias_energy(self.bias_percent / 100)
return counts.energy_mask(emin=e_min)

def make_mask_bkg_peak_energy(self, dataset):

This comment has been minimized.

Copy link
@adonath

adonath Nov 25, 2019

Member

Minor: please rename to make_mask_energy_bkg_peak(), so that the all energy mask methods start with make_mask_energy_xxx

And another minor point: as self is not used you could actually make it @staticmethod

background_spectrum = dataset.background
counts = dataset.counts

peak = background_spectrum.data.max()

This comment has been minimized.

Copy link
@adonath

adonath Nov 25, 2019

Member

This can be simplified using np.argmax() instead.

@adonath adonath self-assigned this Nov 25, 2019
@adonath adonath added the feature label Nov 25, 2019
@adonath adonath added this to To do in gammapy.maps via automation Nov 25, 2019
@adonath adonath added this to the 0.15 milestone Nov 25, 2019
Copy link
Member

adonath left a comment

Thanks @luca-giunti! No further comments from my side...

@adonath adonath merged commit 41e17a4 into gammapy:master Nov 25, 2019
8 of 12 checks passed
8 of 12 checks passed
greeting
Details
gammapy.gammapy #20191125.5 failed
Details
gammapy.gammapy (Test Python36) Test Python36 failed
Details
gammapy.gammapy (Test Windows36) Test Windows36 failed
Details
gammapy.gammapy (Test Windows37) Test Windows37 failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 2 updated code elements – Tests: passed
Details
codecov/patch 91.66% of diff hit (target 91.49%)
Details
codecov/project 91.5% (+<.01%) compared to 2ecaa77
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.maps automation moved this from To do to Done Nov 25, 2019
@adonath adonath changed the title Implement additional methods on the safe mask maker Implement additional methods for SafeMaskMaker Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
2 participants
You can’t perform that action at this time.