Skip to content
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

Add ying yang grid #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ying yang grid #20

wants to merge 1 commit into from

Conversation

nbren12
Copy link
Collaborator

@nbren12 nbren12 commented Dec 13, 2024

This PR adds a new projection based grid, the ying yang grid. It restructures some of the lambert conformal logic a bit, so Simon should take a look.

@nbren12
Copy link
Collaborator Author

nbren12 commented Dec 13, 2024

image


@property
def lat_lon(self):
mesh_x, mesh_y = np.meshgrid(self.x, self.y, indexing='ij')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i needed to change this to use indexing=ij here versus the original which was indexing="xy", since meshgrid has unusual behavior. @simonbyrne please review. Specifically, do we want the data to have the shaped [nx, ny] or [ny,nx]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really have a preference, I think I just copied LatLongGrid which uses Lat (Y) first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need to update the LCC tests?

This PR adds a new projection based grid, the ying yang grid. It
restructures some of the lambert conformal logic a bit, so Simon should
take a look.
pass


class Grid(base.Grid):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just renamed your LCC grid class @simonbyrne . Intended to be used like earth2grid.projections.Grid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if you want to give it a more specific name (i.e. it assumes the underlying grid is rectilinear, not an unstructured mesh).

Copy link
Collaborator Author

@nbren12 nbren12 Dec 13, 2024

Choose a reason for hiding this comment

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

I think "projection" implies rectangle. This is just a style preference for projection.Grid over projection.ProjectionGrid. Same style as healpix.Grid.

pass


class Grid(base.Grid):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if you want to give it a more specific name (i.e. it assumes the underlying grid is rectilinear, not an unstructured mesh).


def vec2ang(x, y, z):
"""convert lon,lat in radians to cartesian coordinates"""
lat = torch.asin(z)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assumes your inputs are already normalized. If you want it to work with non-normalized inputs, you would need

Suggested change
lat = torch.asin(z)
lat = torch.atan2(z, torch.hypot(y, x))


@property
def lat_lon(self):
mesh_x, mesh_y = np.meshgrid(self.x, self.y, indexing='ij')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really have a preference, I think I just copied LatLongGrid which uses Lat (Y) first.


@property
def lat_lon(self):
mesh_x, mesh_y = np.meshgrid(self.x, self.y, indexing='ij')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need to update the LCC tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants