Filter mask

[Aske-Rosted]

adds the option of using I3 files FilterMask to implement a filter when converting files

[RasmusOrsoe]

Hey @Aske-Rosted thanks for this.

Could we add a check before attempting to access the filter quantities that checks it's actually there? Because if its not, the conversion will break. Here is some pseudo-code:

def _filter_mask(
        self, frame: "icetray.I3Frame", I3filters: List[str]
    ) -> bool:
        """Check if specified condition(s) are met.

        Args:
            frame: I3Frame to check.
            I3filters: List of I3Filters to check for pass.
        """
        if "FilterMask" in frame:
              for filter in I3filters:
                  if filter in frame["FilterMask"]:
                          if frame["FilterMask"][filter].condition_passed is False:
                              return True
                  else:
                         self.warning_once(f"Filter '{filter}' not in frame. Filter condition cannot be checked.")
              return False
       else:
             self.warning_once("FilterMask not available in file. Filter conditions cannot be checked.")

[Aske-Rosted]

Does this look good? @RasmusOrsoe

[RasmusOrsoe]

self._filter_mask will return False by default if "FilterMask" not in frame. That means that everyone will be unable to extract i3frames without this field. That's not good!

You can resolve this by either

A) Make _filter_mask return True by default in these cases
B) change if self._filter_mask(frame, self._I3filters) to if self._filter_mask(frame, self._I3filters) & len(self._I3filters) > 0. This way, the frames are only skipped if the user wants to filter out frames this way.

[RasmusOrsoe]

@Aske-Rosted

[Aske-Rosted]

It should very much work such that a True return from the _filter_mask call will skip the frame (through a continue call) while a False return will move on to include the frame. I agree this is a little counter intuitive and should probably be specified in the description.

[RasmusOrsoe]

Sorry. I was fooled. You are right!

[RasmusOrsoe]

Hey! Almost there. Left two comments

[RasmusOrsoe]

You can get rid of the codeclimate error if you let _filter_mask access the i3filters inside the function, instead of passing them as argument.

[Aske-Rosted]

We should probably consider what is the more relevant standard behavior.

Passing all of the included filters is required to keep the frame. or passing 1 of the included filters means keeping the frame.
Also adding the possibility to switch between the two might be necessary.

in the current version it requires passing all the included filters.

[RasmusOrsoe]

We should probably consider what is the more relevant standard behavior.

Passing all of the included filters is required to keep the frame. or passing 1 of the included filters means keeping the frame. Also adding the possibility to switch between the two might be necessary.

in the current version it requires passing all the included filters.

Yes! I think the most elegant solution would be to create a class like so:

from abc import abstractmethod
from graphnet.utilities.logging import Logger
from typing import List

class I3Filter(Logger):
    """A generic Class that defines I3Filter modules."""

    @abstractmethod
    def _skip_frame(self, frame: "icetray.I3Frame") -> bool:
        """A function that defines if the frame should be skipped.
            If the function returns True, the frame is skipped."""
        
    def __call__(self, frame: "icetray.I3Frame") -> bool:
        passing_flag = self._skip_frame(frame)
        try:
            assert isinstance(passing_flag, bool)
        except AssertionError as e:
            self.warning(f"""{self.__class__.__name__} returns an output 
                         of type {type(passing_flag)}. Expected bool.""")
            raise e
        return passing_flag

Our current _skip_frame function in DataConverter (which is essentially also a filter but for NullSplit frames) then becomes a default filter called NullSplitI3Filter:

class NullSplitI3Filter(I3Filter):
    def _skip_frame(self, frame: "icetray.I3Frame") -> bool:
        """Skips all null split frames.

        Args:
            frame: I3Frame to check.

        Returns:
            True if frame is a null split frame, else False.
        """
        if frame["I3EventHeader"].sub_event_stream == "NullSplit":
            return True
        return False

and your new filter based on filtermasks could look like so:

class AskeFilter(I3Filter):
    def __init__(self, filter_masks: List[str]):
        """ A helpful docstring"""
        self._filter_masks = filter_masks
        
    def _skip_frame(self, frame: "icetray.I3Frame") -> bool:
        if "FilterMask" not in frame:
            self.warning_once(
                "FilterMask not found in frame. Skipping filter checks."
            )
            return False
        for filter in self.filter_masks:
            if filter not in frame["FilterMask"]:
                self.warning_once(
                    f"Filter {filter} not found in frame. Skipping."
                )
                continue
            if frame["FilterMask"][filter].condition_passed is False:
                return True
        return False

We can then adjust the arguments to DataConverter like so:

def __init__(
        self,
        extractors: List[I3Extractor],
        outdir: str,
        gcd_rescue: Optional[str] = None,
        *,
        nb_files_to_batch: Optional[int] = None,
        sequential_batch_pattern: Optional[str] = None,
        input_file_batch_pattern: Optional[str] = None,
        workers: int = 1,
        index_column: str = "event_no",
        icetray_verbose: int = 0,
        i3_filters: Union[I3Filter, List[I3Filter]] = [NullSplitI3Filter],
    ):

We can then set the new variable as member of DataConverter:

def __init__(.... i3_filters ..):
      ...
      if isinstance(i3_filters, I3Filter): # If a single filter is passed, turn it into a list
              i3_filters = [i3_filters]
     self._i3_filters = i3_filters

and re-write the _skip_frame in DataConverter to:

def _skip_frame(self, frame: "icetray.I3Frame") -> bool:
        """Evaluates the I3Filters. 
        Events not passing the filters will be skipped

        Args:
            frame: I3Frame to check.

        Returns:
            True if the event does not pass filters, else False.
        """
        skip = False
        for filter in self._i3_filters:
            skip += filter(frame) # False + True = 1
        if skip < len(self._i3_filters): # At least one filter was not passed.
            return True 
        else: 
            return False

This way we have a lot of freedom to define filters as our needs evolve, and we wouldn't have to think to hard about default values as you mention. What do you think @Aske-Rosted?

[Aske-Rosted]

The IceTray unit test is failing due to missing due to [Errno 28] No space left on device which I don't see how is related to these changes. @RasmusOrsoe would you mind giving it a look over?

[RasmusOrsoe]

@Aske-Rosted I re-ran the workflow and now the disk space issue is gone. However, now a new error about missing files is there. Could you double check that you did not by accident cause this?

[Aske-Rosted]

@Aske-Rosted I re-ran the workflow and now the disk space issue is gone. However, now a new error about missing files is there. Could you double check that you did not by accident cause this?

Fixed the unit test issue.

[RasmusOrsoe]

Looks great now. Just a comment on snake-case argument naming. Well done @Aske-Rosted !

[RasmusOrsoe]

We use lower-case "snake-case" by convention. So the argument should be i3_filters: List[I3Filter] = []

[RasmusOrsoe]

self._i3filters

[RasmusOrsoe]

@Aske-Rosted The IceTray disk space error is now fixed in main. Please try to update the branch and see if this doesn't fix the error.

[Aske-Rosted]

@Aske-Rosted The IceTray disk space error is now fixed in main. Please try to update the branch and see if this doesn't fix the error.

hmm new unit test errors and I do not really see how they are related to this change... any thoughts @RasmusOrsoe?

[RasmusOrsoe]

Looks great!