-
Notifications
You must be signed in to change notification settings - Fork 279
feat: Implement true_disp calculation in ctapipe-process + test #2951
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
5845e78
d775870
bea9764
7ed8dec
db1e5ac
348f76e
2f6f3ce
2bb287f
928abe1
1b013b5
df21fae
dc000e4
035297a
fba4463
9d9e010
aa32c83
b157186
6bb1777
66a2994
b44a018
bd7e32c
d1dc7aa
8eeedd3
37603e8
ccc3c6d
43188e7
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Implement calculation of ``true_disp`` parameters (norm and sign) in ``ctapipe-process`` for simulated events. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -483,6 +483,15 @@ class CoreParametersContainer(Container): | |
| psi = Field(nan * u.deg, "Image direction in the Tilted/Ground Frame", unit="deg") | ||
|
|
||
|
|
||
| class TrueDispContainer(Container): | ||
| """ | ||
| Container for true disp parameters | ||
| """ | ||
| default_prefix = "true_disp" | ||
| norm = Field(nan * u.deg, "True disp norm", unit=u.deg) | ||
| sign = Field(nan, "True disp sign") | ||
|
|
||
|
|
||
| class ImageParametersContainer(Container): | ||
| """Collection of image parameters""" | ||
|
|
||
|
|
@@ -520,6 +529,10 @@ class ImageParametersContainer(Container): | |
| default_factory=CoreParametersContainer, | ||
| description="Image direction in the Tilted/Ground Frame", | ||
| ) | ||
| true_disp = Field( | ||
|
Member
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. This is not really an image parameter, so should not be part of this data structure. It rather should be its own subfiled of the
Member
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. It can also directly use the |
||
| default_factory=TrueDispContainer, | ||
| description="True disp parameters (only filled for true_parameters)", | ||
| ) | ||
|
|
||
|
|
||
| class DL1CameraContainer(Container): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,6 +270,8 @@ def _validate_str(self, obj, value): | |
| value = get_dataset_path(value.partition("dataset://")[2]) | ||
| elif url.scheme in ("", "file"): | ||
| value = pathlib.Path(url.netloc, url.path) | ||
| elif os.name == "nt" and len(url.scheme) == 1: | ||
|
Member
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. Please remove this unrelated change from this pull request.
Author
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. Hii, On Windows, I found that traitlets (or the ctapipe wrapper) struggles to validate absolute paths. It misinterprets the drive letter (e.g., C:) as a URL scheme (like http:), which causes validation to fail. I have reverted the fix for now to keep this PR strictly focused on the true_disp implementation, but I wanted to mention it in case it's an issue the team would like addressed in a separate, dedicated PR later.
Member
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. We at the moment explicitly do not support and do not test on windows. |
||
| value = pathlib.Path(value) | ||
| else: | ||
| self.error(obj, value) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -660,6 +660,7 @@ def _write_dl1_telescope_events(self, event: ArrayEventContainer): | |
| true_parameters.concentration, | ||
| true_parameters.morphology, | ||
| true_parameters.intensity_statistics, | ||
| true_parameters.true_disp, | ||
|
Member
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. disp is not an image parameter and does not belong in this group. |
||
| ], | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| # pylint: disable=W0201 | ||
| import sys | ||
|
|
||
| import astropy.units as u | ||
| import numpy as np | ||
| from tqdm.auto import tqdm | ||
|
|
||
| from ..calib import CameraCalibrator, GainSelector | ||
|
|
@@ -30,6 +32,7 @@ | |
| DL2_EVENT_STATISTICS_GROUP, | ||
| ) | ||
| from ..reco import Reconstructor, ShowerProcessor | ||
| from ..reco.preprocessing import horizontal_to_telescope | ||
| from ..utils import EventTypeFilter | ||
|
|
||
| COMPATIBLE_DATALEVELS = [ | ||
|
|
@@ -362,6 +365,45 @@ def start(self): | |
| if self.should_compute_dl1: | ||
| self.process_images(event) | ||
|
|
||
| if event.simulation is not None: | ||
|
Member
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. Please do not put inline computations inside of the process tool. The CLI is a high-level wrapper around ctapipe functionality and should only contain minimal code, certainly not the computations itself. |
||
| shower = event.simulation.shower | ||
| for tel_id, dl1 in event.dl1.tel.items(): | ||
| if ( | ||
| tel_id not in event.simulation.tel | ||
| or event.simulation.tel[tel_id].true_parameters is None | ||
| ): | ||
| continue | ||
|
|
||
| true_param = event.simulation.tel[tel_id].true_parameters | ||
| hillas = dl1.parameters.hillas | ||
|
|
||
| if ( | ||
| hillas is not None | ||
| and np.isfinite(hillas.fov_lat) | ||
| and np.isfinite(hillas.fov_lon) | ||
| ): | ||
| pointing = event.monitoring.tel[tel_id].pointing | ||
| # calculate true disp | ||
| fov_lon, fov_lat = horizontal_to_telescope( | ||
| alt=shower.alt, | ||
| az=shower.az, | ||
| pointing_alt=pointing.altitude, | ||
| pointing_az=pointing.azimuth, | ||
| ) | ||
| # numpy's trigonometric functions need radians | ||
| psi = hillas.psi.to_value(u.rad) | ||
| cog_lon = hillas.fov_lon | ||
| cog_lat = hillas.fov_lat | ||
|
|
||
| delta_lon = fov_lon - cog_lon | ||
| delta_lat = fov_lat - cog_lat | ||
|
|
||
| true_disp = ( | ||
| np.cos(psi) * delta_lon + np.sin(psi) * delta_lat | ||
| ) | ||
| true_param.true_disp.norm = np.abs(true_disp) | ||
| true_param.true_disp.sign = np.sign(true_disp.value) | ||
|
|
||
| if self.should_compute_muon_parameters: | ||
| self.process_muons(event) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
|
|
||
| import numpy as np | ||
| import pandas as pd | ||
| import pytest | ||
| import tables | ||
|
|
||
| from ctapipe.core import run_tool | ||
| from ctapipe.tools.process import ProcessorTool | ||
| from ctapipe.utils import get_dataset_path, resource_file | ||
| from ctapipe.io.hdf5dataformat import SIMULATION_PARAMETERS_GROUP | ||
|
|
||
| def test_true_disp_calculation(tmp_path, dl1_image_file): | ||
| """check true_disp calculation in ctapipe-process""" | ||
| config = resource_file("stage1_config.json") | ||
|
|
||
| output_file = tmp_path / "true_disp_test.dl1.h5" | ||
|
|
||
| run_tool( | ||
| ProcessorTool(), | ||
| argv=[ | ||
| f"--config={config}", | ||
| f"--input={dl1_image_file}", | ||
| f"--output={output_file}", | ||
| "--write-parameters", | ||
| "--overwrite", | ||
| ], | ||
| cwd=tmp_path, | ||
| raises=True, | ||
| ) | ||
|
|
||
| # check if fields exist and are not all NaN | ||
| # We need to find a telescope that has events | ||
|
|
||
| with tables.open_file(output_file, mode="r") as testfile: | ||
| # Check if true parameters group exists for at least one telescope | ||
| sim_params_group = testfile.root.simulation.event.telescope.parameters | ||
| assert len(sim_params_group._v_children) > 0 | ||
|
|
||
| first_tel_group = list(sim_params_group._v_children.keys())[0] | ||
|
|
||
| true_params = pd.read_hdf( | ||
| output_file, f"{SIMULATION_PARAMETERS_GROUP}/{first_tel_group}" | ||
| ) | ||
|
|
||
| assert "true_disp_norm" in true_params.columns | ||
| assert "true_disp_sign" in true_params.columns | ||
|
|
||
| # Check that we have some valid values | ||
| assert np.count_nonzero(np.isfinite(true_params["true_disp_norm"])) > 0 | ||
| assert np.count_nonzero(np.isfinite(true_params["true_disp_sign"])) > 0 |
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.
We store the predicted disp as a single column, not in separate norm and sign columns.
The true disp should be the same.