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.

Sign up for GitHub

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

Fix HillasReconstructor in case of too few telescopes #994

Merged
merged 6 commits into from
Jun 5, 2019
Merged

Fix HillasReconstructor in case of too few telescopes #994

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
399ec78
only counts telescopes with a width that is not np.NaN now
LukasNickel Feb 28, 2019
83b7e8c
using np.isnan to check for nan
LukasNickel Feb 28, 2019
9d16b53
Exception had the wrong telescope number
LukasNickel Feb 28, 2019
a771f56
Raise Exceptions for invalid widths
LukasNickel Mar 11, 2019
7058a23
Adds a test to make sure the HillasReconstructor fails if
LukasNickel May 22, 2019
8450731
adapting the test to account for changes in the master branch (r0 con…
LukasNickel May 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ctapipe/reco/HillasReconstructor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,16 @@ class are set to np.nan

# filter warnings for missing obs time. this is needed because MC data has no obs time
warnings.filterwarnings(action='ignore', category=MissingFrameAttributeWarning)

# stereoscopy needs at least two telescopes
if len(hillas_dict) < 2:

# stereoscopy needs at least two telescopes with valid widths
valid_telescopes = sum([1 if not np.isnan(hillas_dict[x]['width'].value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could directly build a new dict here with all the valid telescopes and only use those later.

E.g. valid_hillas = {tel_id: hillas for tel_id, hillas in hillas_dict.items() if np.isfinite(hillas['width'].value}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be the cleaner approach for now but it kind of comes back to #772.
Do we still use these images by adjusting the weights or do we not use them at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, thanks for digging it out.
My feeling is that 3 aligned pixels still give information about the direction and should not simply be thrown out - but I have not done a study on that ;-)
I see the problem with weights, however, and have no clear fix to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's nice to keep track of what telescopes were dropped. Generally, i think a real pipeline would use a CutFlow to select telescopes before Hillas parameterization (e.g. with a minimum width and length or number of surviving pixels), but the code should still do reasonable things without that separate step, so testing for at least NaN is good. It should only skip telescopes if there is a real problem, and leave it up to the user to make any looser cuts for robustness.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and by real problem I mean e.g. a singular matrix. In that case you could still reconstruct a direction if possible, and set the h_max to NaN

else 0
for x in hillas_dict])

if valid_telescopes < 2:
raise TooFewTelescopesException(
"need at least two telescopes, have {}"
.format(len(hillas_dict)))
.format(valid_telescopes))

self.initialize_hillas_planes(
hillas_dict,
Expand Down