Modify acceptance stacking behavior #5270
Conversation
Signed-off-by: Régis Terrier <rterrier@apc.in2p3.fr>
Signed-off-by: Régis Terrier <rterrier@apc.in2p3.fr>
…ng and needs to be masked first Signed-off-by: Régis Terrier <rterrier@apc.in2p3.fr>
Signed-off-by: Régis Terrier <rterrier@apc.in2p3.fr>
registerrier
added
backport-v1.0.x
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5270 +/- ##
==========================================
+ Coverage 94.25% 94.46% +0.20%
==========================================
Files 234 235 +1
Lines 35226 35496 +270
==========================================
+ Hits 33204 33531 +327
+ Misses 2022 1965 -57 ☔ View full report in Codecov by Sentry. |
| Counts outside the safe mask are lost. | ||
|
|
||
| The ``acceptance`` of the stacked dataset is obtained by stacking the acceptance weighted | ||
| by the other mask_safe onto the current unwieghted acceptance. |
There was a problem hiding this comment.
| by the other mask_safe onto the current unwieghted acceptance. | |
| by the other mask_safe onto the current unweighted acceptance. |
| Note that the masking is not applied to the current dataset. If masking needs | ||
| to be applied to it, use `~gammapy.MapDataset.to_masked()` first. | ||
|
|
||
| The stacked ``acceptance_off`` is scaled so that: |
There was a problem hiding this comment.
Maybe one can put below the formulae for both acceptance, adding the use of the masks (in the `MapDataset.stack' docstring, one uses \epsilon for the mask
There was a problem hiding this comment.
We could instead put that information here https://docs.gammapy.org/1.2/user-guide/makers/ring.html ?
But, no strong opinion on if it should also be in the docstring
There was a problem hiding this comment.
As a user, I think it would definitely be helpful to have the formulae in the user guide (and maybe linked in the docs). Having it in the docstring might be nice too.
There was a problem hiding this comment.
We have all the formulas involved in stacking mentioned here, https://docs.gammapy.org/1.2/user-guide/datasets/index.html#stacking-multiple-datasets
So, maybe updating this and linking to this from the docstring would be useful
| The ``acceptance`` of the stacked dataset is normalized to 1, | ||
| and the stacked ``acceptance_off`` is scaled so that: | ||
| Safe mask is applied to the other dataset to compute the stacked counts data. | ||
| Counts outside the safe mask are lost. |
There was a problem hiding this comment.
I propose to add the formula, as in `MapDataset.stack'
There was a problem hiding this comment.
Thanks @registerrier ! I think the tutorial can be adapted in a separate PR
| Note that the masking is not applied to the current dataset. If masking needs | ||
| to be applied to it, use `~gammapy.MapDataset.to_masked()` first. | ||
|
|
||
| The stacked ``acceptance_off`` is scaled so that: |
There was a problem hiding this comment.
We have all the formulas involved in stacking mentioned here, https://docs.gammapy.org/1.2/user-guide/datasets/index.html#stacking-multiple-datasets
So, maybe updating this and linking to this from the docstring would be useful
Description
This pull request tries to solve #5245 by modifying the way
acceptanceis stacked forMapDatasetOnOff.Now, instead of assuming the total acceptance to be equal to one, the
acceptanceis properly stacked. Becausealphais unchanged
acceptance_offis therefore different now.The PR also corrects an inconsistency noted in #5261 .
MapDataset.stackdoes not apply the mask ofselfwhen stacking. This avoids recomupting empty stacks all the time where in practice the first dataset is always empty.The
MapDataset.OnOffhad a different assumption. This is now made consistent.The docstrings where previously incorrect and have been modified. The formula are probably still incorrect.
Dear reviewer
This is ready, but docstrings need to be checked. Also the tutorial for Ring Background should be updated.