cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
64 stars 268 forks source link

Change `ImageCleaner` API #2511

Closed Hckjs closed 6 months ago

Hckjs commented 7 months ago

Change API of ImageCleaner to be __call__(self, tel_id: int, event: ArrayEventContainer). Closes #2015.

@aleberti

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.53%. Comparing base (3952e5a) to head (8a05e27).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2511 +/- ## ======================================= Coverage 92.53% 92.53% ======================================= Files 235 235 Lines 20063 20064 +1 ======================================= + Hits 18565 18566 +1 Misses 1498 1498 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

maxnoe commented 7 months ago

This change seems to remove the ability to change which image cleaner implementation is used inside it,

Why would something called TailCutsImageCleaner allow to use a different cleaning method than TailCuts?

kosack commented 7 months ago

Why would something called TailCutsImageCleaner allow to use a different cleaning method than TailCuts?

I agree and mentioned that in the request, but that reducer was configurable, presumably for a reason. I suspect more that it was just misnamed, rather than that it should be restricted to one type of cleaning. All it does is iteratively clean and dilate, which should work for any cleaning implementation.

Nevertheless, that should be a different PR if you want to change how it works, and the ACADA people should be involved.

maxnoe commented 7 months ago

Ok...

I think we discussed this already elsewhere: keep the old signature in a method like: clean_low_level (please invent a better name), and have __call__ use that internally.

Then the TailCutsImageReducer (and the TwoStepExtractor) can call the low-level API with minimal changes to them.

Hckjs commented 7 months ago

Thanks for the comments! Adapting it now to have a __call__(self, tel_id: int, event: ArrayEventContainer) which internally uses the abstract clean_image(self, tel_id, image, arrival_times) method defined in each ImageCleaner-subclass.

The TwoStepExtractor is already using the low level tailcuts_clean method.

I'll create another PR for renaming the TailCutsDataVolumeReducer.

maxnoe commented 7 months ago

Maybe a better change would be to for now just add the event as new optional parameter?

def __call__(self, tel_id, image, peak_time=None, *, event=None):

That is

a) backwards compatible b) allows to clean other images than what is in event.dl1.tel[tel_id].image / event.dl1.tel[tel_id].peak_time c) still allows accessing additional information from the event

maxnoe commented 6 months ago

Just to summarize from the discussions that are now in collapsed threads: we decided to not pass the full event, but just the CameraMonitoringContainer, which should have all information necessary for advanced cleaning algorithms.