-
Notifications
You must be signed in to change notification settings - Fork 16
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
Index Mapping Util for Rectangular grids #1258
base: dev
Are you sure you want to change the base?
Changes from all commits
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,168 @@ | ||
"""Utilities for generating ENA maps.""" | ||
|
||
from __future__ import annotations | ||
|
||
import logging | ||
import typing | ||
|
||
import numpy as np | ||
from numpy.typing import NDArray | ||
|
||
from imap_processing.spice import geometry | ||
from imap_processing.ultra.utils import spatial_utils | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
# Ignore linting rule to allow for 6 unrelated args | ||
# Also need to manually specify allowed str literals for order parameter | ||
def match_rect_indices_frame_to_frame( # noqa: PLR0913 | ||
input_frame: geometry.SpiceFrame, | ||
projection_frame: geometry.SpiceFrame, | ||
event_time: float, | ||
input_frame_spacing_deg: float, | ||
projection_frame_spacing_deg: float, | ||
order: typing.Literal["C"] | typing.Literal["F"] = "F", | ||
) -> tuple[NDArray, NDArray, NDArray, NDArray, NDArray, NDArray, NDArray, NDArray]: | ||
""" | ||
Match flattened rectangular grid indices between two reference frames. | ||
|
||
Parameters | ||
---------- | ||
input_frame : geometry.SpiceFrame | ||
The frame in which the input grid is defined. | ||
projection_frame : geometry.SpiceFrame | ||
The frame to which the input grid will be projected. | ||
event_time : float | ||
The time at which to project the grid. | ||
input_frame_spacing_deg : float, optional | ||
The spacing of the input frame grid in degrees, | ||
by default DEFAULT_SPACING_DEG. | ||
projection_frame_spacing_deg : float, optional | ||
The spacing of the projection frame grid in degrees, | ||
by default DEFAULT_SPACING_DEG. | ||
order : str, optional | ||
The order of unraveling/re-raveling the grid points, | ||
by default "F" for column-major Fortran ordering. | ||
See numpy.ravel documentation for more information. | ||
|
||
Returns | ||
------- | ||
tuple[NDArray] | ||
Tuple of the following arrays: | ||
- Ordered 1D array of pixel indices covering the entire input grid. | ||
Guaranteed to have one index for each point on the input grid, | ||
(meaning it will cover the entire input grid). | ||
Size is number of pixels on input grid | ||
`= (az_grid_input.size * el_grid_input.size)`. | ||
- Ordered 1D array of indices in the projection frame | ||
corresponding to each index in the input grid. | ||
Not guaranteed to cover the entire projection grid. | ||
Size is the same as the input indices | ||
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. The use of "input indices" throughout this docstring is confusing. It had me re-reading the input parameter descriptions looking for indices that were input to the function. |
||
= `(az_grid_input.size * el_grid_input.size)`. | ||
- Azimuth values of the input grid points in the input frame, raveled. | ||
Same size as the input indices. | ||
- Elevation values of the input grid points in the input frame, raveled. | ||
Same size as the input indices. | ||
- Azimuth values of the input grid points in the projection frame, raveled. | ||
Same size as the input indices. | ||
- Elevation values of the input grid points in the projection frame, raveled. | ||
Same size as the input indices. | ||
- Azimuth indices of the input grid points in the projection frame az grid. | ||
Same size as the input indices. | ||
- Elevation indices of the input grid points in the projection frame el grid. | ||
Same size as the input indices. | ||
""" | ||
if input_frame_spacing_deg > projection_frame_spacing_deg: | ||
logger.warning( | ||
"Input frame has a larger spacing than the projection frame." | ||
f"\nReceived: input = {input_frame_spacing_deg} degrees " | ||
f"and {projection_frame_spacing_deg} degrees." | ||
) | ||
# Build the azimuth, elevation grid in the inertial frame | ||
az_grid_in, el_grid_in = spatial_utils.build_az_el_grid( | ||
spacing=input_frame_spacing_deg, | ||
input_degrees=True, | ||
output_degrees=False, | ||
centered_azimuth=False, # (0, 2pi rad = 0, 360 deg) | ||
centered_elevation=True, # (-pi/2, pi/2 rad = -90, 90 deg) | ||
)[2:4] | ||
|
||
# Unwrap the grid to a 1D array for same interface as tessellation | ||
az_grid_input_raveled = az_grid_in.ravel(order=order) | ||
el_grid_input_raveled = el_grid_in.ravel(order=order) | ||
|
||
# Get the flattened indices of the grid points in the input frame (Fortran order) | ||
# TODO: Discuss with Nick/Tim/Laura: Should this just be an arange? | ||
flat_indices_in = np.ravel( | ||
np.arange(az_grid_input_raveled.size).reshape(az_grid_in.shape), | ||
order=order, | ||
) | ||
|
||
radii_in = geometry.spherical_to_cartesian( | ||
np.stack( | ||
( | ||
np.ones_like(az_grid_input_raveled), | ||
az_grid_input_raveled, | ||
el_grid_input_raveled, | ||
), | ||
axis=-1, | ||
) | ||
) | ||
|
||
# Project the grid points from the input frame to the projection frame | ||
# radii_proj are cartesian (x,y,z) radii vectors in the projection frame | ||
# corresponding to the grid points in the input frame | ||
radii_proj = geometry.frame_transform( | ||
et=event_time, | ||
position=radii_in, | ||
from_frame=input_frame, | ||
to_frame=projection_frame, | ||
) | ||
|
||
# Convert the (x,y,z) vectors to spherical coordinates in the projection frame | ||
# Then extract the azimuth, elevation angles in the projection frame. Ignore radius. | ||
input_grid_in_proj_spherical_coord = geometry.cartesian_to_spherical( | ||
radii_proj, degrees=False | ||
) | ||
|
||
input_az_in_proj_az = input_grid_in_proj_spherical_coord[:, 1] | ||
input_el_in_proj_el = input_grid_in_proj_spherical_coord[:, 2] | ||
|
||
# Create bin edges for azimuth (0 to 2pi) and elevation (-pi/2 to pi/2) | ||
proj_frame_az_bin_edges, proj_frame_el_bin_edges = spatial_utils.build_az_el_grid( | ||
spacing=projection_frame_spacing_deg, | ||
input_degrees=True, | ||
output_degrees=False, | ||
centered_azimuth=False, # (0, 2pi rad = 0, 360 deg) | ||
centered_elevation=True, # (-pi/2, pi/2 rad = -90, 90 deg) | ||
)[4:6] | ||
|
||
# Use digitize to find indices (-1 since digitize returns 1-based indices) | ||
input_az_in_proj_az_indices = ( | ||
np.digitize(input_az_in_proj_az, proj_frame_az_bin_edges) - 1 | ||
) | ||
input_el_in_proj_el_indices = ( | ||
np.digitize(input_el_in_proj_el, proj_frame_el_bin_edges[::-1]) - 1 | ||
) | ||
|
||
# NOTE: This method of matching indices 1:1 is rectangular grid-focused | ||
# It will not necessarily work with a tessellation (e.g. Healpix) | ||
flat_indices_proj = np.ravel_multi_index( | ||
multi_index=(input_az_in_proj_az_indices, input_el_in_proj_el_indices), | ||
dims=( | ||
int(360 // projection_frame_spacing_deg), | ||
int(180 // projection_frame_spacing_deg), | ||
), | ||
order=order, | ||
) | ||
return ( | ||
flat_indices_in, | ||
flat_indices_proj, | ||
az_grid_input_raveled, | ||
el_grid_input_raveled, | ||
input_az_in_proj_az, | ||
input_el_in_proj_el, | ||
input_az_in_proj_az_indices, | ||
input_el_in_proj_el_indices, | ||
Comment on lines
+166
to
+167
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. Are these indices needed? I'm trying to understand if there is a way to simplify this function signature. Complex function signatures are generally a sign that the function is overly complex, or doing too many things. Also, staying away from 2D indices will be helpful for if/when HEALPix is added. |
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
"""Tests coverage for ENA Map Mapping Util functions.""" | ||
|
||
from unittest import mock | ||
|
||
import numpy as np | ||
import pytest | ||
|
||
from imap_processing.ena_maps import map_utils | ||
from imap_processing.spice import geometry | ||
from imap_processing.ultra.utils import spatial_utils | ||
|
||
|
||
class TestENAMapMappingUtils: | ||
@mock.patch("imap_processing.ena_maps.map_utils.geometry.frame_transform") | ||
def test_match_rect_indices_frame_to_frame_same_frame(self, mock_frame_transform): | ||
# Mock frame transform to return the input position vectors (no transform) | ||
mock_frame_transform.side_effect = ( | ||
lambda et, position, from_frame, to_frame: position | ||
) | ||
et = -1 | ||
spacing_deg = 1 | ||
( | ||
flat_indices_in, | ||
flat_indices_proj, | ||
az_grid_input_raveled, | ||
el_grid_input_raveled, | ||
input_az_in_proj_az, | ||
input_el_in_proj_el, | ||
) = map_utils.match_rect_indices_frame_to_frame( | ||
input_frame=geometry.SpiceFrame.IMAP_DPS, | ||
projection_frame=geometry.SpiceFrame.IMAP_DPS, | ||
event_time=et, | ||
input_frame_spacing_deg=spacing_deg, | ||
projection_frame_spacing_deg=spacing_deg, | ||
)[:6] | ||
|
||
# The two arrays should be the same if the input and projection | ||
# frames are the same and the spacing is the same. | ||
np.testing.assert_array_equal(flat_indices_in, flat_indices_proj) | ||
|
||
# The input and projection azimuth and elevation grids should be the same | ||
np.testing.assert_allclose(az_grid_input_raveled, input_az_in_proj_az) | ||
np.testing.assert_allclose(el_grid_input_raveled, input_el_in_proj_el) | ||
|
||
# The projection azimuth and elevation, when reshaped to a 2D grid, | ||
# should be the same as the input grid created with build_az_el_grid. | ||
np.testing.assert_allclose( | ||
input_az_in_proj_az.reshape( | ||
(180 // spacing_deg, 360 // spacing_deg), order="F" | ||
), | ||
spatial_utils.build_az_el_grid(spacing=np.deg2rad(spacing_deg))[2], | ||
) | ||
np.testing.assert_allclose( | ||
input_el_in_proj_el.reshape( | ||
(180 // spacing_deg, 360 // spacing_deg), order="F" | ||
), | ||
spatial_utils.build_az_el_grid(spacing=np.deg2rad(spacing_deg))[3], | ||
) | ||
|
||
@mock.patch("imap_processing.ena_maps.map_utils.geometry.frame_transform") | ||
@pytest.mark.parametrize("input_spacing_deg", [1 / 4, 1 / 3, 1 / 2, 1]) | ||
def test_match_rect_indices_frame_to_frame_different_spacings( | ||
self, mock_frame_transform, input_spacing_deg | ||
): | ||
# Mock frame transform to return the input position vectors (no transform) | ||
mock_frame_transform.side_effect = ( | ||
lambda et, position, from_frame, to_frame: position | ||
) | ||
|
||
et = -1 | ||
proj_spacing_deg = 1 | ||
|
||
( | ||
flat_indices_in, | ||
flat_indices_proj, | ||
) = map_utils.match_rect_indices_frame_to_frame( | ||
input_frame=geometry.SpiceFrame.IMAP_DPS, | ||
projection_frame=geometry.SpiceFrame.IMAP_DPS, | ||
event_time=et, | ||
input_frame_spacing_deg=input_spacing_deg, | ||
projection_frame_spacing_deg=proj_spacing_deg, | ||
)[:2] | ||
|
||
# The input indices should still be the same length as the projection indices | ||
assert len(flat_indices_in) == len(flat_indices_proj) | ||
|
||
# However, the min and max of the input indices should go from: | ||
# 0 to (180/spacing_deg of the input)*(360/spacing_deg of the input) | ||
# and the min and max of the projection indices should go from: | ||
# 0 to (180/spacing_deg of the projection)*(360/spacing_deg of the projection) | ||
assert flat_indices_in.min() == 0 | ||
assert flat_indices_in.max() == ( | ||
(180 / (input_spacing_deg)) * (360 / (input_spacing_deg)) - 1 | ||
) | ||
assert flat_indices_proj.min() == 0 | ||
assert ( | ||
flat_indices_proj.max() | ||
== ((180 / proj_spacing_deg) * (360 / proj_spacing_deg)) - 1 | ||
) | ||
|
||
# There should be (proj_spacing_deg/input_spacing_deg)**2 | ||
# instances of each projection index, and proj_spacing_deg = 1. | ||
assert np.all(np.bincount(flat_indices_proj) == 1 / (input_spacing_deg**2)) |
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.
I think that this function is doing too much...
To make it more general, flexible, and reusable, I would like to see the following function extracted.
The whole push-pull idea is likely going to be specific to Ultra, so avoiding baking that in at this stage would be good. The function that builds the grids will likely want to be moved into this common map_utils module, though that can be in another PR.
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.
Okay, to confirm, you do see there being some overarching function that does basically this, with its components broken out a bit into sub-functions?
For Ultra, I think we want to be able to hand a single function input+output frames, and input+output spacings, and have it match up the indices, which does involve all these steps, with the exception of reshaping back to 2D, which doesn't currently happen here. I think you're referencing the
ravel_multi_index
, which is needed to convert from two small 1D indices (projected az bin, projected el bin) to a single 1D index of the 'unwrapped' sky.Agreed! Is there anywhere in this code currently that talks about push/pull, or did you mean this as more of a precaution?
Got it - I made this ticket #1260 . Is it "cleaner" to do so in another PR? It seems messy to me to have all these PRs interacting with the same code, so I'd lean towards doing it here, but maybe that makes reviewing more difficult?
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.
I'm not sure about an overarching function. For example, this function as written will regenerate the Map coordinates (azimuth, elevation, and pixel edges) for each PSET that you are binning. It would be good to avoid that by generating that once at the start of L2 processing and reuse throughout. Likewise, for the coordinate grid of each PSET, does Ultra store those in the PSET product? or do they really need to be regenerated in this function?
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.
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.
Got it! I think I agree in principle with the goal of not re-generating the map coordinates for each PSET that gets added, but that relies on there already being something like a map object set up, and I wonder if we're edging closer to just implementing that, when we had been talking about a more focused set of self-contained utils for starting Ultra L2.
The L1c files will have the separate coordinates (i.e the 1D grids of az, el), but the
np.meshgrid
would need to be constructed and unwrapped. I'm not sure I see the harm in regenerating these relatively lightweight grids.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 is true. I think that things are going in the right direction here and common, reusable code is being identified. The reason I identify this re-generation of coordinates as an issue is not due to inefficiencies/performance reasons, it is because it becomes more difficult to disentangle down the road and is inflexible. Specifically, I have two concerns:
What happens if Ultra decides they need to change the PSET grid spacing at some point in time or need different spacings for different energy bands. L2 needs to get the grid spacing from the PSET product so that different resolution products can be combined.Looking at the code again I realize this could be handled with the current implementation.