Muon parameters update #2670
Muon parameters update #2670
Conversation
There was a problem hiding this comment.
Hi @jstvdk
Thanks a lot for preparing this. This looks like a good starting point. I have a couple of first comments mainly about the names of the fields and their descriptions.
I will do second round on more technical issues of the code later.
src/ctapipe/containers.py
Outdated
| @@ -1189,6 +1189,18 @@ class MuonParametersContainer(Container): | |||
| nan * u.deg**2, | |||
| "MSE of the deviation of all pixels after cleaning from the ring fit", | |||
| ) | |||
| ring_size = Field(nan, "Number of pixels in the ring") | |||
There was a problem hiding this comment.
Is this really number of pixels? The code below computes some of the pixel intensities.
If this is actually the sum of pixel charges, the name should be ring_intensity and the help string match what is actually computed.
src/ctapipe/containers.py
Outdated
| @@ -1189,6 +1189,18 @@ class MuonParametersContainer(Container): | |||
| nan * u.deg**2, | |||
| "MSE of the deviation of all pixels after cleaning from the ring fit", | |||
| ) | |||
| ring_size = Field(nan, "Number of pixels in the ring") | |||
| size_outside = Field(nan, "Number of pixels outside the ring") | |||
There was a problem hiding this comment.
same here: probably this should be intensity_outside_ring
src/ctapipe/containers.py
Outdated
| @@ -1189,6 +1189,18 @@ class MuonParametersContainer(Container): | |||
| nan * u.deg**2, | |||
| "MSE of the deviation of all pixels after cleaning from the ring fit", | |||
| ) | |||
| ring_size = Field(nan, "Number of pixels in the ring") | |||
| size_outside = Field(nan, "Number of pixels outside the ring") | |||
| num_pixels_in_ring = Field(nan, "Number of pixels in the ring") | |||
There was a problem hiding this comment.
We use the n_ prefix for variables that are numbers, not num_
src/ctapipe/containers.py
Outdated
| ring_size = Field(nan, "Number of pixels in the ring") | ||
| size_outside = Field(nan, "Number of pixels outside the ring") | ||
| num_pixels_in_ring = Field(nan, "Number of pixels in the ring") | ||
| mean_pixel_outside_ring = Field(nan, "Mean pixel charge outside the ring") |
There was a problem hiding this comment.
there is an intensity or similar missing in the field name.
I'd call it mean_intensity_outside_ring
src/ctapipe/containers.py
Outdated
| mean_pixel_outside_ring = Field(nan, "Mean pixel charge outside the ring") | ||
| standard_dev = Field( | ||
| nan * u.deg, | ||
| "Standard deviation of the light distribution along the ring radius.", |
There was a problem hiding this comment.
"along the ring radius" sounds like this is somehow using spatial information.
But I think this is just "of pixels on the ring", right?
| image_mask, | ||
| ) | ||
|
|
||
| assert ring_size > 0 |
There was a problem hiding this comment.
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
src/ctapipe/image/muon/features.py
Outdated
| y0 = center_y.to_value(u.deg) | ||
| pix_x = pixel_x.to_value(u.deg) | ||
| pix_y = pixel_y.to_value(u.deg) | ||
| pixel_r = np.sqrt((pix_x - x0) ** 2 + (pix_y - y0) ** 2) |
There was a problem hiding this comment.
| pixel_r = np.sqrt((pix_x - x0) ** 2 + (pix_y - y0) ** 2) | |
| pixel_r = np.hypot(pix_x - x0, pix_y - y0) |
src/ctapipe/image/muon/features.py
Outdated
| x0 = center_x.to_value(u.deg) | ||
| y0 = center_y.to_value(u.deg) | ||
| pix_x = pixel_x.to_value(u.deg) | ||
| pix_y = pixel_y.to_value(u.deg) |
There was a problem hiding this comment.
I don't think you need to convert and take the value here. Numpy works fine with unit quantities, and then the function will work no matter what unit the inputs are in.
This comment has been minimized.
This comment has been minimized.
| 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 ring, but still close to it | ||
| Mean intensity of the pixels outside the integration area of the ring, |
There was a problem hiding this comment.
Is this the same as l1201 of ctapipe.containers? If so, why the docstrings are different?
There was a problem hiding this comment.
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 from ctapipe.image.muon.features?
There was a problem hiding this comment.
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.
Perhaps this function simply should return
MuonParametersContainer?
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 of MuonParametersContainer. Should I reconsider this and return calculated parts of MuonParametersContainer from every method?
Also, the number of input parameters is very high, consider to wrap them into a structure (dict, container, ...)
Good point
This comment has been minimized.
This comment has been minimized.
| x coordinates of the camera pixels. | ||
| pixel_y : array-like | ||
| y coordinates of the camera pixels. | ||
| Amplitude of image pixels. |
| ) | ||
|
|
||
|
|
||
| def radial_light_distribution(center_x, center_y, pixel_x, pixel_y, image): |
There was a problem hiding this comment.
Consider merging this and the above functions, implementing along the way unification of the return type.
There was a problem hiding this comment.
Sorry, let me repeat if I understood correctly. You are proposing to merge all the methods into one, which calculates and returns MuonParametersContainer?
This comment has been minimized.
This comment has been minimized.
1 similar comment
Analysis Details0 Issues
Coverage and Duplications
Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs |
I added new functionality to the muon analysis as described in the Issue #2665 , specifically
MuonParametersContainerctapipe/image/muon/features.pyctapipe/image/muon/tests/test_muon_features.pyctapipe/image/muon/processor.pyto process and save new muon parametersAdded changes mainly follow existing code in
lstchain/image/muon/muon_analysis.py, with one exception:num_pixels_in_ring- now it takes into account only pixels that survived the cleaning, contrary to what currently implemented inlstchain, where this variable contains all pixels inside the geometrical area of the ring.