Skip to content

Fix HillasIntersection error for badly reconstructed events#2265

Merged
maxnoe merged 7 commits into
cta-observatory:mainfrom
gschwefer:hillasintersection_fix
Sep 6, 2023
Merged

Fix HillasIntersection error for badly reconstructed events#2265
maxnoe merged 7 commits into
cta-observatory:mainfrom
gschwefer:hillasintersection_fix

Conversation

@gschwefer
Copy link
Copy Markdown
Contributor

Fix for issue #2264. Events with large reconstructed angular distances from the camera center now return the INVALID container.

Comment thread ctapipe/reco/hillas_intersection.py Outdated
# Catch events reconstructed at great angular distance from camera center
# and retrun INVALID container to prevent SkyCoord error below.
if (
np.abs(src_fov_lat) * u.rad > 45 * u.deg
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd define the 45 ° limit in a named constant on top, something like FOV_ANGULAR_DISTANCE_LIMIT = 45 * u.deg and then compare to FOV_ANGULAR_DISTANCE_LIMIT.to_value(u.rad) instead of multiplying both sides with the unit (faster).

Maybe you can also put this part into a function so it looks like this:

if far_outside_fov(src_fov_lat, src_fov_lon):
    return INVALID

Copy link
Copy Markdown
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Could you add a test for this case? I.e. by constructing an ArrayEvent with specific values in the HillasParameterContainer that triggers this issue?

@gschwefer gschwefer requested a review from maxnoe March 8, 2023 12:43
Comment thread ctapipe/reco/hillas_intersection.py Outdated
FOV_ANGULAR_DISTANCE_LIMIT = 45 * u.deg


def far_outside_fov(fov_lat, fov_lon):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd make this private (i.e. add an underscore in front)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, no problem

Comment thread ctapipe/reco/hillas_intersection.py Outdated
Copy link
Copy Markdown
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Looks good, aside from two minor comments regarding performance and visibility

@gschwefer
Copy link
Copy Markdown
Contributor Author

Thanks! I will implement them and get back to you.

@gschwefer gschwefer linked an issue Mar 8, 2023 that may be closed by this pull request
Comment thread ctapipe/reco/hillas_intersection.py
@maxnoe maxnoe requested review from kosack and maxnoe March 13, 2023 12:44
@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Mar 13, 2023

@gschwefer Could you add a change-log entry?

Just create a file docs/changes/2265.bugfix.rst with a short description

@maxnoe
Copy link
Copy Markdown
Member

maxnoe commented Sep 4, 2023

@gschwefer Can you merge or rebase with the current main branch to get the correct CI definition so we can get this in?

@gschwefer
Copy link
Copy Markdown
Contributor Author

Yes I will

@gschwefer gschwefer force-pushed the hillasintersection_fix branch from f751de2 to 2557709 Compare September 6, 2023 06:30
@maxnoe maxnoe merged commit 27494ca into cta-observatory:main Sep 6, 2023
@kosack kosack added the bug label Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HillasIntersection produces error on badly reconstructed events

3 participants