Skip to content

Fix periodicity behavior of chord_length#2873

Merged
maxnoe merged 1 commit into
mainfrom
fix-chord-length-periodicity
Oct 24, 2025
Merged

Fix periodicity behavior of chord_length#2873
maxnoe merged 1 commit into
mainfrom
fix-chord-length-periodicity

Conversation

@maxnoe
Copy link
Copy Markdown
Member

@maxnoe maxnoe commented Oct 24, 2025

In [1]: from ctapipe.image.muon.intensity_fitter import chord_length
In [2]: import numpy as np
In [3]: phi = np.linspace(0, 2 * np.pi, 500)
In [4]: %timeit chord_length(10.0, 1.2, phi)

This branch:

5.6 μs ± 12.5 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

On branch of #2872:

7.6 μs ± 28.2 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
from ctapipe.image.muon.intensity_fitter import chord_length
import numpy as np
import matplotlib.pyplot as plt


phi = np.linspace(-4 * np.pi, 4 * np.pi, 1000)

fig, ax = plt.subplots(1, 1, layout="constrained", sharex=True)

for rho in (0.5, 1.2):
    ax.plot(phi, chord_length(10, rho, phi), label=f"{rho = }")

ax.legend()
fig.savefig("chord_length.png")
plt.show()

Before:
chord_length

Now:
chord_length

@maxnoe maxnoe force-pushed the fix-chord-length-periodicity branch 2 times, most recently from 2ac1053 to a69ce02 Compare October 24, 2025 11:26
@maxnoe maxnoe force-pushed the fix-chord-length-periodicity branch from a69ce02 to c069237 Compare October 24, 2025 11:32
@burmist-git
Copy link
Copy Markdown
Member

So the modulo involves really expensive calculations… But are you sure that this replacement of the modulo works for all cases?

@maxnoe
Copy link
Copy Markdown
Member Author

maxnoe commented Oct 24, 2025

It passes the tests... and logically, yes, it should be the same as we use the cosine to select the same domain as the modulus.

@burmist-git
Copy link
Copy Markdown
Member

It passes the tests... and logically, yes, it should be the same as we use the cosine to select the same domain as the modulus.

Yes one test is passing. I am trusting you but this check you are doing is not intuitive for me. I will approve the PR. But will make more tests.

@maxnoe
Copy link
Copy Markdown
Member Author

maxnoe commented Oct 24, 2025

But will make more tests.

More tests are always welcome! Just make sure to add them to the repo :)

@ctao-sonarqube
Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

@maxnoe
Copy link
Copy Markdown
Member Author

maxnoe commented Oct 24, 2025

from ctapipe.image.muon.intensity_fitter import chord_length
import numpy as np
import matplotlib.pyplot as plt


phi = np.linspace(-4 * np.pi, 4 * np.pi, 1000)
rho = 1.2

phi_mod = (phi + np.pi) % (2 * np.pi) - np.pi
mod_cond = np.abs(phi_mod) < np.arcsin(1 / rho)
new_cond = (np.abs(np.sin(phi)) < 1 / rho) & (np.cos(phi) > 0)

fig, ax = plt.subplots(1, 1, layout="constrained", sharex=True)

ax.plot(phi, mod_cond, label='mod condition')
ax.plot(phi, new_cond, ls=':', label='cos condition')
ax.legend()

fig.savefig("chord_conditions.png")
chord_length

@maxnoe maxnoe merged commit 67de23e into main Oct 24, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants