Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/changes/2969.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add chord length calculation for one or more polygons in the primary mirror description for muon analysis.
166 changes: 166 additions & 0 deletions src/ctapipe/image/muon/intensity_fitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,172 @@
SQRT2 = np.sqrt(2)


def polygon_chord(mu_x, mu_y, phi, vertices_list):
"""
Compute the total chord length through one or more polygons for a set of
projection angles.

For each angle in ``phi``, the function evaluates the chord length
contributed by every polygon in ``vertices_list`` using
``polygon_chord_base`` and returns the summed chord length.

Parameters
----------
mu_x : float
X-coordinate of the ray origin, which is the muon's impact point (x).
mu_y : float
Y-coordinate of the ray origin, which is the muon's impact point (y).
phi : array-like
Angle defining the direction of the ray (cher. photon from muon).
vertices_list : list of ndarray
List of polygons. Each polygon is represented as an ``(N, 2)`` array
containing the polygon vertices in order.

Returns
-------
numpy.ndarray
One-dimensional array containing the total chord length for each
projection angle in ``phi``.

Notes
-----
Each polygon is processed independently, and the resulting chord lengths
are summed for every projection angle.
"""

ri_x = []
ri_y = []
vi_x = []
vi_y = []

for ver_i in vertices_list:
ver_f = np.roll(ver_i, 1, axis=0)

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.

In my experience, np.roll is extremely expensive, especially when called in a loop here, as it makes full copies of its input data.

In general I am much worried about the (runtime) performance of the code here, as it contains a lot of raw python loops without numba compilation for functions that are meant to be called in a likelihood optimization.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For the optimization, let’s leave it for the next iteration. The Numba optimization can be applied to cases with zero, one, or two intersection points. However, for more complex shapes, such as the one we want to have for a the LST mirror (not the single facet), this requires a sorting function that cannot be used with Numba.

ri_x.append(ver_i[:, 0])
ri_y.append(ver_i[:, 1])
vi_x.append(ver_f[:, 0] - ver_i[:, 0])
vi_y.append(ver_f[:, 1] - ver_i[:, 1])

the_chord = []
for az in phi:
chord_l = 0.0
for i in np.arange(len(vertices_list)):
chord_l += polygon_chord_base(
mu_x, mu_y, az, ri_x[i], ri_y[i], vi_x[i], vi_y[i]
)

the_chord.append(chord_l)

return np.array(the_chord)


def polygon_chord_base(

Check failure on line 100 in src/ctapipe/image/muon/intensity_fitter.py

View check run for this annotation

CTAO Sonarqube / SonarQube Code Analysis

Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.

[S3776] Cognitive Complexity of functions should not be too high See more on https://sonar-ctao.zeuthen.desy.de/project/issues?id=cta-observatory_ctapipe_6122e87b-83f3-4db1-8287-457e752adf01&pullRequest=2969&issues=b1c6fc6d-c144-4455-b8c9-2c8d93a28eb8&open=b1c6fc6d-c144-4455-b8c9-2c8d93a28eb8
mu_x, mu_y, phi, ri_x, ri_y, vi_x, vi_y, return_intersections=False
):
"""
Compute the chord length of a ray intersecting a polygon.

This function calculates the length of the segment formed by the intersection
of a ray (defined by an origin and direction) with a polygon defined by its
edges. The polygon is represented parametrically using starting points and
direction vectors for each edge.

Parameters
----------
mu_x : float

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These should be quantities, as we discussed offline, the best would be to use telescope coordinate system lat/lon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, perhaps tuples of (x,y) or arrays of them would be more straightforward to define points (arrays of points)

X-coordinate of the ray origin, which is the muon's impact point (x).
mu_y : float
Y-coordinate of the ray origin, which is the muon's impact point (y).
phi : float
Angle defining the direction of the ray.
ri_x : ndarray of shape (N,)
X-coordinates of the starting points of the polygon edges.
ri_y : ndarray of shape (N,)
Y-coordinates of the starting points of the polygon edges.
vi_x : ndarray of shape (N,)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should deduce these (if needed) from the vertices. You can write a helper for this.

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.

Yes, I'd also prefer the vertices as input. But maybe the function as it is here is fine and that transformation is made once outside of this function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes agree with @maxnoe . This is a low-level function. Let's keep it as it is, since all the necessary computations should be handled by the higher-level function that calls it.

X-components of the edge direction vectors.
vi_y : ndarray of shape (N,)
Y-components of the edge direction vectors.

Returns
-------
float

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be also a unit of length (or angular, equivalent in the telescope frame)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Every branch returns (length, x_int, y_int) and not only a float. This should be reflected in the docstring.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is indeed only a float. The function is not vectorized.

Length of the chord formed by the intersection of the ray with the polygon:
- 0.0 if there is no intersection,
- distance from the ray origin to the intersection point if only one intersection,
- distance between two intersection points if exactly two intersections,
- accumulated segment length for multiple intersections (handles complex polygons).

Notes
-----
- The ray is defined parametrically as:
(x, y) = (mu_x, mu_y) + s * (cos(phi), sin(phi)), with s >= 0
- Each polygon edge is defined as:
(x, y) = (ri_x, ri_y) + t * (vi_x, vi_y), with 0 <= t < 1
- The function computes intersections by solving a 2D linear system.
- A small epsilon_d (`1e-20`) is added to the denominator to avoid division by zero.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not using

try:
  ...
except ZeroDivisionError:
  # determinant is zero, the cord goes on the edge, handle this

@kosack kosack Apr 13, 2026

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.

Just remember that catching an exception is quite a bit slower than adding an epsilon, so if you need this to be fast, the epsilon method might be better. Also if you want to use numba to speed things up

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.

And doesn't vectorize

- For multiple intersections, distances are sorted and combined with alternating
signs to compute the total chord length (useful for non-convex polygons).

"""

# Effective speed of the ray, with unit norm.
vmu_x = np.cos(phi)
vmu_y = np.sin(phi)

epsilon_d = 1.0e-20

c1 = mu_x - ri_x
c2 = mu_y - ri_y
determinant = vi_x * vmu_y - vi_y * vmu_x + epsilon_d

t = (c1 * vmu_y - c2 * vmu_x) / determinant
s = (vi_y * c1 - vi_x * c2) / determinant

status = np.column_stack((vi_x, ri_x, vi_y, ri_y, t, s))

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.

Why do you column_stack this (making a copy) just to then index into the individual comments making it basically unreadable what you are accessing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, thank you. It can be removed. Initially, I did it to have everything in the same 2D array, but you’re right, it can be removed.

mask = (status[:, 4] >= 0) & (status[:, 4] < 1) & (status[:, 5] >= 0)

x_int = status[mask][:, 0] * status[mask][:, 4] + status[mask][:, 1]
y_int = status[mask][:, 2] * status[mask][:, 4] + status[mask][:, 3]

if x_int.shape[0] == 0:
if return_intersections:
return 0.0, np.nan, np.nan
return 0.0
elif x_int.shape[0] == 1:
if return_intersections:
return (
np.squeeze(np.sqrt((x_int - mu_x) ** 2 + (y_int - mu_y) ** 2)),
np.squeeze(x_int),
np.squeeze(y_int),
)
return np.squeeze(np.sqrt((x_int - mu_x) ** 2 + (y_int - mu_y) ** 2))

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.

please do not duplicate computation of the same things in different branches.

compute first, then return, i.e.

chord_length = ...
if return intersections:
    return chord_length, ...
return chord_length

elif x_int.shape[0] == 2:
if return_intersections:
return (
np.squeeze(
np.sqrt((x_int[0] - x_int[1]) ** 2 + (y_int[0] - y_int[1]) ** 2)
),
np.squeeze(x_int),
np.squeeze(y_int),
)
return np.squeeze(
np.sqrt((x_int[0] - x_int[1]) ** 2 + (y_int[0] - y_int[1]) ** 2)
)
else:
dist = np.sort(np.squeeze(np.sqrt((x_int - mu_x) ** 2 + (y_int - mu_y) ** 2)))
sign_arr = np.ones(x_int.shape[0])
if x_int.shape[0] % 2 == 0:
sign_arr[0::2] = -1
if return_intersections:
return np.sum(dist * sign_arr), np.squeeze(x_int), np.squeeze(y_int)
return np.sum(dist * sign_arr)

sign_arr[1::2] = -1
if return_intersections:
return np.sum(dist * sign_arr), np.squeeze(x_int), np.squeeze(y_int)
return np.sum(dist * sign_arr)


def chord_length(radius, rho, phi, phi0=0):
"""
Function for integrating the length of a chord across a circle (effective chord length).
Expand Down
132 changes: 132 additions & 0 deletions src/ctapipe/image/muon/tests/test_intensity_fit.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,138 @@
import pytest
from scipy.constants import alpha


@pytest.mark.parametrize(
"muon_x, muon_y, photon_phi, expected_chord_length",
[
(0.0, 200.0, 90.0 * u.deg, 75.0),
(0.0, -200.0, 150.0 * u.deg, 75.0),
(0.0, 0.0, 90.0 * u.deg, 225.0),
(0.0, -400.0, 90.0 * u.deg, 450.0),
(0.0, 0.0, 30.0 * u.deg, 75.0),
(0.0, -200.0, 0.0 * u.deg, 86.6025 * 3),
(200.0, 200.0, 0.0 * u.deg, 0.0),
],
)
def test_polygon_chord(muon_x, muon_y, photon_phi, expected_chord_length):
from ctapipe.image.muon.intensity_fitter import polygon_chord

ver_a = np.array(
[
[86.6025, 0.0],
[43.3013, 75.0],
[-43.3013, 75.0],
[-86.6025, 0.0],
[-43.3013, -75.0],
[43.3013, -75.0],
]
)
ver_b = ver_a + [[0.0, 200.0]]
ver_c = ver_a + [[0.0, -200.0]]
ver_d = ver_a + [[200.0, -200.0]]

assert np.isclose(
polygon_chord(
muon_x,
muon_y,
np.array([photon_phi.to_value(u.rad)]),
[ver_a, ver_b, ver_c, ver_d],
),
expected_chord_length,
atol=0.001,
)


@pytest.mark.parametrize(
"ver_initial, muon_x, muon_y, photon_phi, expected_chord_length",
[
(
np.array(
[
[86.6025, 0.0],
[43.3013, 75.0],
[-43.3013, 75.0],
[-86.6025, 0.0],
[-43.3013, -75.0],
[43.3013, -75.0],
]
),
0.0,
0.0,
90.0 * u.deg,
75.0,
),
(
np.array(
[
[86.6025, 0.0],
[43.3013, 75.0],
[-43.3013, 75.0],
[-86.6025, 0.0],
[-43.3013, -75.0],
[43.3013, -75.0],
]
),
0.0,
-200.0,
90.0 * u.deg,
150.0,
),
(
np.array(
[
[86.6025, 0.0],
[43.3013, 75.0],
[-43.3013, 75.0],
[-86.6025, 0.0],
[-43.3013, -75.0],
[43.3013, -75.0],
]
),
0.0,
0.0,
30.0 * u.deg,
75.0,
),
(
np.array(
[
[86.6025, 0.0],
[43.3013, 75.0],
[-43.3013, 75.0],
[-86.6025, 0.0],
[-43.3013, -75.0],
[43.3013, -75.0],
]
),
200.0,
200.0,
30.0 * u.deg,
0.0,
),
],
)
def test_polygon_chord_base(
ver_initial, muon_x, muon_y, photon_phi, expected_chord_length
):
from ctapipe.image.muon.intensity_fitter import polygon_chord_base

ver_final = np.roll(ver_initial, 1, axis=0)
assert np.isclose(
polygon_chord_base(
muon_x,
muon_y,
photon_phi.to_value(u.rad),
ri_x=ver_initial[:, 0],
ri_y=ver_initial[:, 1],
vi_x=ver_final[:, 0] - ver_initial[:, 0],
vi_y=ver_final[:, 1] - ver_initial[:, 1],
),
expected_chord_length,
atol=0.001,
)


parameter_names = [
"radius",
"rho",
Expand Down
Loading