Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a shower processor #1675

Merged
merged 36 commits into from
May 21, 2021

Conversation

HealthyPear
Copy link
Member

@HealthyPear HealthyPear commented Apr 8, 2021

This pull request implements a ShowerProcessor, similar to the previously implemented ImageProcessor.

Shower geometry reconstruction is performed with HillasReconstructor only

It could be generalized in principle to all Reconstructor-based classes, but that class needs to upgraded accordingly and it will probably trigger a different discussion related to e.g. the relation between something like HillasReconstructor and ImPACT.

I am open to integrate this PR with something like a reconstructor_type attribute to ShowerProcessor (like image_cleaner_type for ImageProcessor), but this would require at least to make ctapipe.reco.ImPACTReconstructor not a class based on Reconstructor.

ShowerProcessoras the name says is supposed to process the shower interpreted as an event

Currently when ShowerProcessor is called the shower geometry is always reconstructed, being the minimum operation we can do on parametrized images.

It will be also possible to reconstruct energy and particle type, so for the moment, I added them as dummy functions that raises a NotImplementedError.

Unit-test

The unit-test tests for the following cases:

  • successful reconstruction
  • unsuccessful reconstruction due to not enough DL1b instances
  • call of the currently unsupported energy or particle type reconstructions

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1675 (8ef9f5c) into master (5a874ec) will increase coverage by 0.03%.
The diff coverage is 98.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1675      +/-   ##
==========================================
+ Coverage   90.83%   90.87%   +0.03%     
==========================================
  Files         183      185       +2     
  Lines       14299    14370      +71     
==========================================
+ Hits        12989    13059      +70     
- Misses       1310     1311       +1     
Impacted Files Coverage Δ
ctapipe/core/qualityquery.py 97.82% <50.00%> (-2.18%) ⬇️
ctapipe/reco/__init__.py 100.00% <100.00%> (ø)
ctapipe/reco/shower_processor.py 100.00% <100.00%> (ø)
ctapipe/reco/tests/test_shower_processor.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a874ec...8ef9f5c. Read the comment docs.

ctapipe/reco/shower_processor.py Outdated Show resolved Hide resolved
@maxnoe
Copy link
Member

maxnoe commented Apr 8, 2021

@kosack, yes that was what I was essentially also proposing above. Building the hillas dict like:

hillas_dict = {
    tel_id: dl1.parameters.hillas
    for tel_id, dl1 in event.dl1.tel.items()
    if all(self.check_event(dl1.parameters))
}

@kosack
Copy link
Contributor

kosack commented May 20, 2021

Yes, this is also something I wanted to change. I think it would be good to expose also the single steps in the I mageProcessor and test them as public API. Not just the call.
Similar for the steps in Stage1, so pipelines can easier add custom behaviour by inheriting from stage1 and overriding / adding some methods.

Before we do that I would prefer to collect some use cases to see what another pipeline would really want to override. The current private API is in my opinion not really designed for that purpose, so just making it public is not the best design.

@HealthyPear
Copy link
Member Author

So for now it's @kosack 1 - @maxnoe 1 !

@HealthyPear HealthyPear requested review from maxnoe and kosack May 20, 2021 12:26
@maxnoe
Copy link
Member

maxnoe commented May 20, 2021

@HealthyPear I think we can go ahead here independent of that and talk about what needs to be adapted / changed to have a more fine-grained public API later / when I will continue with trying to port the full LST analysis to vanilla ctapipe.

@HealthyPear
Copy link
Member Author

@HealthyPear I think we can go ahead here independent of that and talk about what needs to be adapted / changed to have a more fine-grained public API later / when I will continue with trying to port the full LST analysis to vanilla ctapipe.

Sure for me it's also fine, I do not have preferences.
From the point of view of protopipe I think the ShowerProcessor will be a black box after initialization (unless bugs or design problems arise, which is still possible but that's the reason we have protopipe to test...).

I do not have further modifications unless you or Karl have other comments.

kosack
kosack previously approved these changes May 20, 2021
ctapipe/reco/__init__.py Outdated Show resolved Hide resolved
@HealthyPear HealthyPear requested review from maxnoe and kosack May 20, 2021 18:39
@kosack kosack merged commit 3cd627a into cta-observatory:master May 21, 2021
@HealthyPear HealthyPear deleted the feature-shower_processor branch July 5, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants