introducing union support in ObservationFilter #4616
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
MRegeard
force-pushed
the
logic_obs_filter
branch
from
415990f
to
255da27
Compare
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
bkhelifi
reviewed
Jun 30, 2023
| @@ -23,8 +26,10 @@ class ObservationFilter: | |||
| class that corresponds to the filter type | |||
| (see `~gammapy.data.ObservationFilter.EVENT_FILTER_TYPES`) | |||
|
|
|||
| The filtered event list will be an intersection of all filters. A union | |||
| of filters is not supported yet. | |||
| event_filter_method: str | |||
There was a problem hiding this comment.
Should we add an example with this argument in the docstring exemple?
bkhelifi
reviewed
Jun 30, 2023
| @@ -102,6 +102,13 @@ def test_filter_gti(observation): | |||
| "p_in": [], | |||
| "p_out": 1, | |||
| }, | |||
| { | |||
| "p_in": [ | |||
There was a problem hiding this comment.
Should we add other test with an union and an intersection?
|
What is the status here @MRegeard ? |
|
@registerrier, I think that the logic that I introduce here is not satisfactory. That's why I didn't went further. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduce "union" filtering in
ObservationFilter, as mentioned in #4511.First of all, I introduce a new input parameter to the class,
event_filter_method, which can take either the value'intersect'or'union'. By default it is set to'intersect'to stick to the former behaviour.Then I modified the
filter_eventsmethod to be able to do "union filtering". For that, I take the event filtered by each filter one by one,vstacktheir table, sort the table by'TIME', applyuniqueto table and then put this in a newEventListwhich is retuned.I also modified the
_check_filter_phase(that check for "phase filter" and apply the livetime correction if needed) so that it can support multiple filtering.Finally, I also added a staticmethod
_check_overlap_phase, which check if there are overlap in the definebandof "phase filter". This is mandatory to avoid having livetime that are not corresponding to the really fraction of livetime that is kept.