I think that this function is doing too much...

• building both az_el grids
• flattening
• doing transforms
• generating indices for mapping both directions
• reshaping back to 2D

To make it more general, flexible, and reusable, I would like to see the following function extracted.

def map_2d_coords_to_pixels(coords: NDArray, pixel_edges: Tuple(array_like), order: typing.Literal["C"] | typing.Literal["F"] = "F") -> NDArray:
    """
    Compute the flat indices that map the input coordinates to the pixels defined by pixel_edges.

    Parameters
    -----------
    coords : ndarray with shape (N, 2) where N is the number of input coordinate pairs.
    pixel_edges: Tuple[ndarray] where pixel_edges[0] defines the 0th axis pixel edges...
    order: order to use when converting from tuple of index arrays into an array of flat indices

    Returns
    --------
    indices : ndarray
        Flat array of indices with size = N that map the input coordinates into an array of pixels defined by the input pixel_edges that has been flattened with the same `order` as input.
    """

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.

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.

The whole push-pull idea is likely going to be specific to Ultra, so avoiding baking that in at this stage would be good.

Agreed! Is there anywhere in this code currently that talks about push/pull, or did you mean this as more of a precaution?

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.

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?

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 (except reshaping back to 2D, which doesn't 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.

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?

Agreed! Is there anywhere in this code currently that talks about push/pull, or did you mean this as more of a precaution?
You're right. There is no push pull in this function. I wasn't careful enough reading the descriptions of the outputs.

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.

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?

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.

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.

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.
• Similarly, Hi and Lo have a 1D spatial array in the PSET products rather than 2D and don't, AFAIK, have any need for pull binning. With that in mind, there is talk of needing to combine Hi and Ultra data, so assumptions about the input PSET dimensionality need to be removed as much as possible.

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.

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.