creare-com / podpac

Pipeline for Observational Data Processing Analysis and Collaboration
https://podpac.org
Apache License 2.0
45 stars 6 forks source link

Add Coordinate Resolution #505

Closed CFoye-Creare closed 1 year ago

CFoye-Creare commented 1 year ago

Description

New feature branch to address issue #502. We are looking to implement a helper function to return the resolution of coordinate systems at different levels of accuracy.

Proposed Changes

For a Coordinate object, a method horizontal_resolution to return the resolution of horizontal coordinate system. This should support both stacked and unstacked coordinates. We are offering three kinds of resolutions:

CFoye-Creare commented 1 year ago

Currently assumes the inputted data will be in valid lat/lon degrees

CFoye-Creare commented 1 year ago
mpu-creare commented 1 year ago

Could you also edit the MR and add a short description of the changes so we can remember what they were next time we look at this. Also add a link to the relevant issue.

CFoye-Creare commented 1 year ago

See https://github.com/creare-com/podpac/issues/502#issuecomment-1425955172 for some info on how I implemented the unstacked coords this time.

jmilloy commented 1 year ago

I haven't been following this but I get all the notificatons and took a brief look today. I'm not going to make a thorough review, but I notice that this new method has logical cases for different types of coordinates which is something I tried pretty hard to avoid when developing the coordinates module. So something like this would be more in line with the style of the rest of the module:

def horizontal_resolution(self, units, type):
    resolution_dict = {}
    for dim, c in self.items():
        if 'lat' in c.dims or 'lon' in c.dims:
            resolution_dict[dim] = c.horizontal_resolution(units, type)

or maybe

def horizonal_resolution(self, units, type):
    return {dim: c.horizontal_resolution(units, type) for dim, c in self.items() if c.is_horizontal}

Then the four methods are defined each in Coordinates1d or StackedCoordinates instead. Similarly, I think there are some benefits to then testing these methods individually in test_coordinates1d and test_stacked_coordinates and then lightly testing the Coordinates horizontal_resolution class that pulls them together. Anyways, just a note.

mpu-creare commented 1 year ago

@jmilloy this would typically work but to get the resolution you need both the latitude and longitude, even in the unstacked case -- so it can't be handled in Coordinates1d. Unless you have a better suggestion?

I really do appreciate the note on the design.

jmilloy commented 1 year ago

@mpu-creare Yeah, I wondered about that and wasn't thorough enough. I'd consider passing the lat in, which would ignored in some cases, but make it very clear what's needed.

for dim, c in self.items():
    resolution[dim] = c.horizontal_resolution(units=units, type=type, lat=self['lat'])

In fact, you can technically make Coordinates with a lon dimension but not a lat dimension, right? And then calling horizontal_resolution on coordinates without a 'lat' should be caught and raised as an error, right? I didn't look super closely but I don't think it is. I think making it it explicit helps point that out and reminds you to test for it.

jmilloy commented 1 year ago

(Also, don't use type as an argument/variable)

CFoye-Creare commented 1 year ago

@jmilloy restype work?

CFoye-Creare commented 1 year ago

Wrote a check for unstacked "lat" or unstacked "lon" without a counterpart. Now throwing a ValueError. Also deleted some lingering commented code, and changed the "type" parameter to "restype".

CFoye-Creare commented 1 year ago

I realized that the lat value actually matters for longitude as well, because the distances are greater near the equator due to the bump at the equator.

mpu-creare commented 1 year ago

@CFoye-Creare Nice first PR. Thanks for navigating all the trickiness behind this implementation. Merging.

Closes #502 .