Skip to content

Support telescope position from EarthLocations for SubarrayDescription #2424

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
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
22 changes: 16 additions & 6 deletions ctapipe/instrument/subarray.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ def __init__(
----------
name : str
name of this subarray
tel_positions : Dict[int, np.ndarray]
dict of x,y,z telescope positions on the ground by tel_id. These are
tel_positions : Dict[int, Union[np.ndarray, EarthLocation]]
dict of x,y,z telescope positions on the ground by tel_id or EarthLocation. These are
Copy link
Member

Choose a reason for hiding this comment

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

by tel_id or EarthLocation sounds like EarthLocation could be the key of the dictionary, not another format for the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, better with something like

dict of telescope positions by tel_id, given as x,y,z position or EarthLocation.

converted internally to a coordinate in the `~ctapipe.coordinates.GroundFrame`
tel_descriptions : Dict[TelescopeDescription]
dict of TelescopeDescriptions by tel_id
Expand All @@ -91,7 +91,9 @@ def __init__(
coordinate system used for `tel_positions`.
"""
self.name = name
self.positions = tel_positions or dict()
self.positions: Dict[int, Union[np.ndarray, EarthLocation]] = (
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't use mixed types for internal storage. We should accept EarthLocation or GroundFrame coordinates for the inputs, but then convert in __init__ to one common format.

I think for now, that should be the previous, so ground frame coordinas.

tel_positions or dict()
)
self.tels: Dict[int, TelescopeDescription] = tel_descriptions or dict()
self.reference_location = reference_location

Expand Down Expand Up @@ -157,9 +159,17 @@ def info(self, printer=print):
def tel_coords(self):
"""Telescope positions in `~ctapipe.coordinates.GroundFrame`"""

pos_x = [p[0].to_value(u.m) for p in self.positions.values()]
pos_y = [p[1].to_value(u.m) for p in self.positions.values()]
pos_z = [p[2].to_value(u.m) for p in self.positions.values()]
positions = self.positions.values()
Copy link
Member

Choose a reason for hiding this comment

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

See above, internal representation should not be mixed.

positions = [
GroundFrame.from_earth_location(p, self.reference_location).cartesian.xyz
if isinstance(p, EarthLocation)
else p
for p in positions
]

pos_x = [p[0].to_value(u.m) for p in positions]
pos_y = [p[1].to_value(u.m) for p in positions]
pos_z = [p[2].to_value(u.m) for p in positions]

frame = GroundFrame(reference_location=self.reference_location)

Expand Down
38 changes: 38 additions & 0 deletions ctapipe/instrument/tests/test_subarray.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,3 +287,41 @@ def test_subarrays(subarray_prod5_paranal: SubarrayDescription):
assert subarray.name == "NewArray"
assert isinstance(subarray.reference_location, EarthLocation)
assert subarray.reference_location == subarray_prod5_paranal.reference_location


def test_tel_pos_from_EarthLocation(prod5_mst_nectarcam):
rng = np.random.default_rng(0)

pos = {}
tel = {}

for tel_id in range(1, 11):
tel[tel_id] = prod5_mst_nectarcam
pos[tel_id] = rng.uniform(-100, 100, size=3) * u.m

for tel_id in range(12, 23):
tel[tel_id] = prod5_mst_nectarcam
rnd_lon = rng.uniform(
LOCATION.lon.to_value("deg") - 1e-3, LOCATION.lon.to_value("deg") + 1e-3
)
rnd_lat = rng.uniform(
LOCATION.lat.to_value("deg") - 1e-3, LOCATION.lat.to_value("deg") + 1e-3
)
rnd_height = rng.uniform(
LOCATION.height.to_value("m") - 1e2, LOCATION.height.to_value("m") + 1e2
)
pos[tel_id] = EarthLocation(
lon=rnd_lon * u.deg, lat=rnd_lat * u.deg, height=rnd_height
)

subarray = SubarrayDescription(
"test array",
tel_positions=pos,
tel_descriptions=tel,
reference_location=LOCATION,
)

x, y, z = subarray.tel_coords.cartesian.xyz
assert all(x.value > -100) and all(x.value < 100)
assert all(y.value > -100) and all(y.value < 100)
assert all(z.value > -100) and all(z.value < 100)