Change API of ImageCleaner to be __call__(self, event: ArrayEvent)

[maxnoe]

Please describe the use case that requires this feature.
Currently, the ImageCleaner has the API of __call__(self, tel_id, image, peak_time).
For more advanced cleaning methods that e.g. require monitoring information about the current NSB / pedestal variation, it is impossible to use this information.

Describe the solution you'd like

Also switch to __call__(self, event) so the cleaning algorithms can access all relevant information.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context

E.g. LSTs cleaning is using a factor of the pedestal standard deviation as a lower bound for the cleaning threshold.

[maxnoe]

Side note, I think this shows again that we should Invert the in memory container hierarchy as discussed in #1165 to have

ArrayEvent
    TelescopeEvent
         r1
         dl1
         dl2
         mon

Instead of telescope maps for each data level.

[kosack]

I don't really like passing too much info to low-level algorithms as it makes it hard to follow what they actually use, and the code becomes needsly confusing (this was one of the "lessons learned" we tried to avoid in ctapipe). Since I think pedestal variances are clearly useful for image cleaning, I would prefer just to pass those to all image cleaners explicitly in ImageProcessor.

for the discussion about the array structure, see #1165

[maxnoe]

I think it is important here to make a difference between low-level code in small, self-contained functions with well-defined inputs and outputs and processing components that need to be flexible to allow access to more information that might be needed and which we currently do not anticipate.

E.g. the low-level tail_cuts_clean function would stay as it is, but the Component should get the ArrayEvent (or TelescopeEvent once we implement what is proposed in CEP 2 / #1165) so that the levels can be adapted to the monitoring info before passing to the tailcuts clean.

We have seen time and again, that more information becomes useful or is needed and expanding the API every time that happens is I think worse than just providing the high level API with the full event container.

[maxnoe]

See also comment in this issue which I closed as duplicate of this one:
#2331 (comment)

[kosack]

There was a reason it was implemented this way: reusability. The idea was that you might have an ImageCleaner component in multiple places, for example inside the TwoPassImageExtractor, inside the DataVolumeReducer and later (as now) inside the ImageProcessor. Each would get it's image and peak time from a different place, so it was cleaner (no pun intended) to not hard-code that in the __call__ method.

You can of course still use low-level versions of those in each place, but having a configurable algorithm has advantanges: e.g. we could study which cleaning method works best for DVR without having to change the code, or even use different methods for different cameras/telescopes

[maxnoe]

You are now arguing some hypothetical use cases against a real one that was brought up multiple times.

A usecase that is so important to e.g. the analysis of LST commissioning data, that lack of support for it prevents moving to using the ctapipe tools for the analysis.

We need to enable this.

Since the components can be configured, I don't see a reason why we can't add the needed configuration options needed for the use cases you mentioned even if we change to the more general API.

Another possibility would be to have two public methods in ImageCleaner: __call__(self, ArrayEvent) and a lower-level clean(tel_id, image, peak_time).

[HealthyPear]

Regarding #2331, would it be possible to temporarily add a MAGICCleaner under ctapipe/image/cleaning.py which does not inherit from ImageCleaner and gets called with the event container?

In this way, we ensure that the current structure of ctapipe cleaning API does not get modified yet, but we allow MAGIC people to update their code bases with more recent versions of ctapipe (I guess we can assume that a hypothetical MAGICCleaner is used only by MAGIC-related projects for now).

Then when this issue gets closed you would only need to change the class parent (I am assuming very lightly).

[aleberti]

Any update on this? Now magic-cta-pipe moved to ctapipe v0.19, and I would really like to be able to move to use ctapipe tools for the pipeline, at least at the low level. Right now I am still stuck because I cannot implement the MAGIC cleaning inheriting directly from ImageCleaner. If there is another solution, of course let me know.