-
Notifications
You must be signed in to change notification settings - Fork 272
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
Muon parameters update #2670
base: main
Are you sure you want to change the base?
Muon parameters update #2670
Changes from all commits
6d55373
a1ab58a
385e039
dca7ff8
69b496a
a54658a
d2677d9
87831c1
d1924ad
68d291b
81fecaa
6140d30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,3 +158,125 @@ def ring_containment(radius, center_x, center_y, camera_radius): | |
|
||
a = (radius**2 - camera_radius**2 + d**2) / (2 * d) | ||
return np.arccos(a / radius) / np.pi | ||
|
||
|
||
def ring_size_parameters( | ||
radius, | ||
center_x, | ||
center_y, | ||
pixel_x, | ||
pixel_y, | ||
ring_integration_width, | ||
outer_ring_width, | ||
image, | ||
image_mask, | ||
): | ||
""" | ||
Calculate the parameters related to the size of the ring image. | ||
|
||
Parameters | ||
---------- | ||
radius: float | ||
radius of the ring | ||
center_x: float | ||
x coordinate of the ring center | ||
center_y: float | ||
y coordinate of the ring center | ||
pixel_x: array-like | ||
x coordinates of the camera pixels | ||
pixel_y: array-like | ||
y coordinates of the camera pixels | ||
ring_integration_width: float | ||
Width of the ring in fractions of ring radius | ||
outer_ring_width: float | ||
Width of the outer ring in fractions of ring radius | ||
image: array-like | ||
Amplitude of image pixels | ||
image_mask: array-like | ||
mask of the camera pixels after cleaning | ||
|
||
Returns | ||
------- | ||
ring_intensity: float | ||
Sum of the p.e. inside the integration area of the ring | ||
intensity_outside_ring: float | ||
Sum of the p.e. outside the ring integration area that passed the cleaning | ||
n_pixels_in_ring: int | ||
Number of pixels inside the ring integration area that passed the cleaning | ||
mean_intensity_outside_ring: float | ||
Mean intensity of the pixels outside the integration area of the ring, | ||
and restricted by the outer ring width, i.e. in the strip between | ||
ring integration width and outer ring width. | ||
""" | ||
|
||
dist = np.sqrt((pixel_x - center_x) ** 2 + (pixel_y - center_y) ** 2) | ||
dist_mask = np.abs(dist - radius) < (radius * ring_integration_width) | ||
pix_ring = image * dist_mask | ||
pix_outside_ring = image * ~dist_mask | ||
|
||
dist_mask_2 = np.logical_and( | ||
~dist_mask, | ||
np.abs(dist - radius) < radius * (ring_integration_width + outer_ring_width), | ||
) | ||
pix_ring_2 = image[dist_mask_2] | ||
|
||
ring_intensity = np.sum(pix_ring) | ||
intensity_outside_ring = np.sum(pix_outside_ring * image_mask) | ||
n_pixels_in_ring = np.sum(dist_mask & image_mask) | ||
mean_intensity_outside_ring = np.sum(pix_ring_2) / len(pix_ring_2) | ||
|
||
return ( | ||
ring_intensity, | ||
intensity_outside_ring, | ||
n_pixels_in_ring, | ||
mean_intensity_outside_ring, | ||
) | ||
|
||
|
||
def radial_light_distribution(center_x, center_y, pixel_x, pixel_y, image): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider merging this and the above functions, implementing along the way unification of the return type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, let me repeat if I understood correctly. You are proposing to merge all the methods into one, which calculates and returns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
""" | ||
Calculate the radial distribution of the muon ring. | ||
|
||
Parameters | ||
---------- | ||
center_x : float | ||
x coordinate of the ring center. | ||
center_y : float | ||
y coordinate of the ring center. | ||
pixel_x : array-like | ||
x coordinates of the camera pixels. | ||
pixel_y : array-like | ||
y coordinates of the camera pixels. | ||
Amplitude of image pixels. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "image" is missing? |
||
|
||
Returns | ||
------- | ||
standard_dev : `astropy.units.Quantity` | ||
Standard deviation of the light distribution in degrees. | ||
Spread of pixel intensities around the mean radial distance from the ring center. | ||
|
||
skewness : float | ||
Skewness of the radial light distribution. | ||
Measures the asymmetry of the distribution around the mean radius. | ||
|
||
excess_kurtosis : float | ||
Excess kurtosis of the radial light distribution. | ||
Indicates the "tailedness" of the distribution compared to a normal distribution. | ||
""" | ||
|
||
if np.sum(image) == 0: | ||
return np.nan * u.deg, np.nan, np.nan | ||
|
||
pixel_r = np.hypot(pixel_x - center_x, pixel_y - center_y) | ||
|
||
mean = np.average(pixel_r, weights=image) | ||
delta_r = pixel_r - mean | ||
standard_dev = np.sqrt(np.average(delta_r**2, weights=image)) | ||
skewness = np.average(delta_r**3, weights=image) / standard_dev**3 | ||
excess_kurtosis = np.average(delta_r**4, weights=image) / standard_dev**4 - 3.0 | ||
|
||
return ( | ||
standard_dev, | ||
skewness.to_value(u.dimensionless_unscaled), | ||
excess_kurtosis.to_value(u.dimensionless_unscaled), | ||
) |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -3,7 +3,12 @@ | |||
import astropy.units as u | ||||
import numpy as np | ||||
|
||||
from ctapipe.image.muon.features import ring_completeness, ring_containment | ||||
from ctapipe.image.muon.features import ( | ||||
radial_light_distribution, | ||||
ring_completeness, | ||||
ring_containment, | ||||
ring_size_parameters, | ||||
) | ||||
|
||||
|
||||
def test_ring_containment(): | ||||
|
@@ -50,3 +55,70 @@ def test_ring_completeness(): | |||
|
||||
assert ring_comp <= 1 | ||||
assert ring_comp >= 0 | ||||
|
||||
|
||||
def test_ring_size_parameters(): | ||||
radius = 1 | ||||
center_x = 0 | ||||
center_y = 0 | ||||
pixel_x = np.linspace(-2, 2, 1855) | ||||
pixel_y = np.linspace(-2, 2, 1855) | ||||
ring_integration_width = 0.25 | ||||
outer_ring_width = 0.2 | ||||
image = np.random.normal(loc=100, scale=10, size=1855) | ||||
image_mask = np.random.choice([True, False], size=1855) | ||||
|
||||
( | ||||
ring_size, | ||||
size_outside, | ||||
num_pixels_in_ring, | ||||
mean_pixel_outside_ring, | ||||
) = ring_size_parameters( | ||||
radius, | ||||
center_x, | ||||
center_y, | ||||
pixel_x, | ||||
pixel_y, | ||||
ring_integration_width, | ||||
outer_ring_width, | ||||
image, | ||||
image_mask, | ||||
) | ||||
|
||||
assert ring_size > 0 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this test is not definitive enough. Please consider writing a test actually being sensitive on changes other than the value being <= 0. I.e. use the ring gaussian toymodel to create a ring with known properties: ctapipe/src/ctapipe/image/toymodel.py Line 375 in 7858ddc
|
||||
assert size_outside > 0 | ||||
assert num_pixels_in_ring > 0 | ||||
assert mean_pixel_outside_ring > 0 | ||||
|
||||
|
||||
def test_radial_light_distribution(): | ||||
center_x = 0.0 * u.deg | ||||
center_y = 0.0 * u.deg | ||||
pixel_x = np.linspace(-10, 10, 1855) * u.deg | ||||
pixel_y = np.linspace(-10, 10, 1855) * u.deg | ||||
image = np.random.normal(loc=100, scale=10, size=1855) | ||||
|
||||
standard_dev, skewness, excess_kurtosis = radial_light_distribution( | ||||
center_x, center_y, pixel_x, pixel_y, image | ||||
) | ||||
|
||||
assert standard_dev.unit == u.deg | ||||
assert np.isfinite(standard_dev.value) | ||||
assert np.isfinite(skewness) | ||||
assert np.isfinite(excess_kurtosis) | ||||
|
||||
|
||||
def test_radial_light_distribution_zero_image(): | ||||
center_x = 0.0 * u.deg | ||||
center_y = 0.0 * u.deg | ||||
pixel_x = np.linspace(-10, 10, 1855) * u.deg | ||||
pixel_y = np.linspace(-10, 10, 1855) * u.deg | ||||
image = np.zeros(1855) | ||||
|
||||
standard_dev, skewness, excess_kurtosis = radial_light_distribution( | ||||
center_x, center_y, pixel_x, pixel_y, image | ||||
) | ||||
|
||||
assert np.isnan(standard_dev) | ||||
assert np.isnan(skewness) | ||||
assert np.isnan(excess_kurtosis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same as l1201 of
ctapipe.containers
? If so, why the docstrings are different?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are the same, but I didn't want to duplicate the exact same text.
Or should I just duplicate the doc strings from
ctapipe.containers
into the doc strings in the methods fromctapipe.image.muon.features
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this function simply should return
MuonParametersContainer
?Also, the number of input parameters is very high, consider to wrap them into a structure (dict, container, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this module in fact is returning just one
MuonParametersContainer
and I adopted already existing style where each method returning some logically/technically connected part ofMuonParametersContainer
. Should I reconsider this and return calculated parts ofMuonParametersContainer
from every method?Good point