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 a SafeMaskMaker at DL3 level #4599

Merged
merged 11 commits into from Jan 24, 2024
Merged

Conversation

AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Jun 20, 2023

This PR implements a SafeMaskMaker to run on DL3 IRFs as discussed in #3878 .
The cuts are applied directly on the event list.
Since HAWC and SWGO are directly using DL4 IRFs, I guess there is no need to adapt this PR for non pointing instruments.

In this case, the SafeMakerDL3 must be run before the MapDatasetMaker, as opposed to the SafeMaskMaker

The energy dispersion is not applied as yet.

@AtreyeeS AtreyeeS added this to To do in gammapy.makers via automation Jun 20, 2023
@AtreyeeS AtreyeeS added this to the 1.2 milestone Jun 20, 2023
@AtreyeeS AtreyeeS linked an issue Jun 20, 2023 that may be closed by this pull request
Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AtreyeeS . This is interesting, but I think we have to understand how this fits in the workflow.

A few general comments/questions:

  • technically this is more an event filtering scheme than a mask creation algorithm. One could use the existing ObservationFilter.
  • one can probably split event filtering from mask creation, but the latter is still necessary for further treatment and analysis.
  • the advantage of creating the masks on the input geometry and apply them to filter events is that you don't have to worry about fractional exposures in the bins. If not, mask_safe has to contain weights between 0 and 1 rather than booleans. This might be useful in a number of situation but requires heavy modifications in stacking and other functionalities. Currently edisp is the place where we do this (and once stacked, the mask_safe is partly redundant with the EdispKernelMap).
  • The idea of having event filtering as part of the list of Makers is interesting because it could allow reading observations from the DataStore within the loop over observations (or in that case obs_id).

One option could be first to adapt the SafeMaskMaker to use the observation IRF rather than the Dataset IRF to build the mask. This would alleviate some of the difficulties we have currently.

Opinions @adonath @QRemy ?

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (921f1a9) 75.71% compared to head (8b75dc5) 75.67%.
Report is 96 commits behind head on main.

Files Patch % Lines
gammapy/makers/safe.py 4.68% 61 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4599      +/-   ##
==========================================
- Coverage   75.71%   75.67%   -0.05%     
==========================================
  Files         228      229       +1     
  Lines       33778    33865      +87     
==========================================
+ Hits        25576    25627      +51     
- Misses       8202     8238      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AtreyeeS
Copy link
Member Author

This should be ready for review now

@QRemy QRemy self-requested a review December 20, 2023 11:21
@QRemy QRemy self-assigned this Dec 20, 2023
Copy link
Contributor

@QRemy QRemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AtreyeeS, I left few minor comments. It may be further simplified to have less nested if tests, but if it is not possible it may be nicer to move the DL3/DL4 cases in separate functions. I will have a closer look into that.

Edit: I removed a bit of code duplication, @AtreyeeS, @registerrier could you look back to it ?

gammapy/makers/safe.py Outdated Show resolved Hide resolved
gammapy/makers/safe.py Outdated Show resolved Hide resolved
gammapy/makers/safe.py Outdated Show resolved Hide resolved
gammapy/makers/safe.py Outdated Show resolved Hide resolved
AtreyeeS and others added 5 commits December 20, 2023 17:09
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Signed-off-by: Atreyee Sinha <atreyee@Debjyotis-Mac-mini.local>
Signed-off-by: Atreyee Sinha <atreyee@Debjyotis-Mac-mini.local>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
Signed-off-by: Quentin Remy <quentin.remy@mpi-hd.mpg.de>
registerrier
registerrier previously approved these changes Jan 17, 2024
Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AtreyeeS . This looks good. I have only minor comments.

@@ -84,6 +88,10 @@ def __init__(
"`position` and `fixed_offset` attributes are mutually exclusive"
)

if irfs not in ["DL3", "DL4"]:
raise IOError("Invalid option fro irfs. Choose 'DL3' or 'DL4'.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise IOError("Invalid option fro irfs. Choose 'DL3' or 'DL4'.")
raise ValueError("Invalid option for irfs: expected 'DL3' or 'DL4', got {irfs} instead.")

gammapy/makers/safe.py Outdated Show resolved Hide resolved
Comment on lines 50 to 51
Whether to use reprojected ("DL4") or raw ("DL3") irfs
Default is "DL4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Whether to use reprojected ("DL4") or raw ("DL3") irfs
Default is "DL4"
Whether to use reprojected ("DL4") or raw ("DL3") irfs.
Default is "DL4".

gammapy/makers/safe.py Outdated Show resolved Hide resolved
gammapy/makers/safe.py Outdated Show resolved Hide resolved
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
gammapy.makers automation moved this from To do to In progress Jan 19, 2024
AtreyeeS and others added 2 commits January 19, 2024 15:18
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
gammapy/makers/safe.py Outdated Show resolved Hide resolved
@AtreyeeS
Copy link
Member Author

Thanks @registerrier ! Commited your suggestions. Should be good to merge now

@AtreyeeS AtreyeeS closed this Jan 19, 2024
gammapy.makers automation moved this from In progress to Done Jan 19, 2024
@AtreyeeS AtreyeeS reopened this Jan 19, 2024
gammapy.makers automation moved this from Done to In progress Jan 19, 2024
Signed-off-by: Atreyee Sinha <asinha@ucm.es>
Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AtreyeeS . No further comment.

@registerrier
Copy link
Contributor

CI fail is unrelated. Merging now.

@registerrier registerrier merged commit a711d11 into gammapy:main Jan 24, 2024
12 of 16 checks passed
gammapy.makers automation moved this from In progress to Done Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Safe mask functions should not rely on the projected IRFs
3 participants