Improve ShowerProcessor API, fixes #2071

[StFroese]

I changed two major things.

1. ShowerProcessor.__call__ now no longer fills the Containers from the Reconstructors into the event structure. This is now done directly in the call function of each Reconstructor
2. ShowerProcessor now allows a list of multiple reconstructors. I also had to change the ProcessorTool to write event statistic tables for a list of reconstructors.

I also changed almost every config in the resources folder and some tests. Please take a look at that 😅

[codecov]

Codecov Report

Base: 92.43% // Head: 92.49% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (f09ff2a) compared to base (b0d9b33).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2073      +/-   ##
==========================================
+ Coverage   92.43%   92.49%   +0.05%     
==========================================
  Files         197      197              
  Lines       16198    16206       +8     
==========================================
+ Hits        14972    14989      +17     
+ Misses       1226     1217       -9     

Impacted Files	Coverage Δ
ctapipe/reco/hillas_intersection.py	93.67% <100.00%> (+0.03%)	⬆️
ctapipe/reco/hillas_reconstructor.py	98.38% <100.00%> (+0.01%)	⬆️
ctapipe/reco/reco_algorithms.py	100.00% <100.00%> (+3.03%)	⬆️
ctapipe/reco/shower_processor.py	100.00% <100.00%> (ø)
ctapipe/reco/tests/test_HillasReconstructor.py	100.00% <100.00%> (ø)
ctapipe/reco/tests/test_hillas_intersection.py	100.00% <100.00%> (ø)
ctapipe/reco/tests/test_shower_processor.py	100.00% <100.00%> (ø)
ctapipe/tools/process.py	92.22% <100.00%> (+0.08%)	⬆️
ctapipe/io/tests/test_event_source.py	96.87% <0.00%> (+1.14%)	⬆️
ctapipe/io/eventsource.py	90.75% <0.00%> (+1.68%)	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kosack]

In light of this change, we should also review #2052 and perhaps consider making ImPACTReconstructor a subclass of Reconstructor again, and then it could be included in this list. It is dependent on a previous reconstructor, so some check would be needed to ensure that the user didn't forget to include it's seed reconstructor.

[kosack]

I notice we still have a discrepancy in the output sign of the azimuth... (probably just a quadrant difference, so maybe not a problem, but still could lead to confusion):

[maxnoe]

Small comment, otherwise looks good. (And happy that this was as easy as I thought)

[StFroese]

In light of this change, we should also review #2052 and perhaps consider making ImPACTReconstructor a subclass of Reconstructor again, and then it could be included in this list. It is dependent on a previous reconstructor, so some check would be needed to ensure that the user didn't forget to include it's seed reconstructor.

Perhaps we can give the base Reconstructor class an attribute self.required_reconstructors that defaults to None and in case of ImPACT or some other reconstructor contains a list of the required reconstructor names. We can check if the names are part of self.reconstructor_types inside the __init__ of the ShowerProcessor and otherwise raise an exception.
This could be done in this PR.

[maxnoe]

I'd rather have the higher level reconstructors have traitlets for which reconstructions to use as input and them raising errors if they cannot find it I think.

Something like this:

class ImPACT(Reconstructor):
     geometry_seed = Unicode(default_value='HillasReconstructor').tag(config=True)
     energy_seed = ...

    def __call__(self, event):
         if self.geometry_seed not in event.dl2.stereo.geometry:
             raise ValueError()

Like this, the reconstructor makes the checks, also when used on its own.

[StFroese]

So it results in the config that you suggested in the issue. Great, that makes sense to me.

[maxnoe]

@StFroese Can you resolve the conflict? You might need to change more things than the conflict implies to get the correct behavior, since you moved the code.

[StFroese]

@StFroese Can you resolve the conflict? You might need to change more things than the conflict implies to get the correct behavior, since you moved the code.

Yes

[maxnoe]

@StFroese Can you remove all the "HillasIntersection" things from the test configs where not explicitly wanted?

We shouldn't spend too much time in the CI making duplicate reconstructions

[StFroese]

@StFroese Can you remove all the "HillasIntersection" things from the test configs where not explicitly wanted?

We shouldn't spend too much time in the CI making duplicate reconstructions

Done