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 to_latlon method #3

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

sandorkertesz
Copy link
Collaborator

@sandorkertesz sandorkertesz commented Mar 25, 2024

image

constants.south,
int((constants.north - constants.south) / dy) + 1,
)
lon_v = np.linspace(0, constants.full_circle - dx, int(constants.full_circle / dx))
Copy link
Member

Choose a reason for hiding this comment

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

This "constants.full_circle - dx" doesn't cover the cases not integer-divisible to 360 (like 0.7, 0.13, 0.14, ...). You're looking for np.arange I believe

Copy link
Member

Choose a reason for hiding this comment

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

You might want to have control over endpoint (that linspace allows direct control, but arange doesn't unfortunately)

Copy link
Collaborator Author

@sandorkertesz sandorkertesz Mar 25, 2024

Choose a reason for hiding this comment

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

This "constants.full_circle - dx" doesn't cover the cases not integer-divisible to 360 (like 0.7, 0.13, 0.14, ...). You're looking for np.arange I believe

I think we should use the same algorithm here that MIR uses. E.g. when generating a 0.7x0.7 global grid this is how the resulting grid looks like:

image

Copy link
Member

Choose a reason for hiding this comment

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

Yes indeed -- and I believe that np.arange can get you there faster :-) You will however "need to count" the resulting number of points (also, as mir has to do when understanding the increments + bounding box

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 managed to reproduce the same lat/lons that mir generates, will push the code soon.

from . import constants


def generate_latlon(dx, dy):
Copy link
Member

Choose a reason for hiding this comment

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

I would rename these (dx,dy) to (dlon,dlat)

from . import constants


def generate_latlon(dx, dy):
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the name should be a bit more explicit that it will generate complete matrices of lats/lons rather than a 1d array of each. Something like generate_latlon_grids or generate_latlon_matrices (still trying to think of the better name)

Copy link
Collaborator Author

@sandorkertesz sandorkertesz Mar 25, 2024

Choose a reason for hiding this comment

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

The primary goal of this method is to return the latitudes/longitudes of the global grid generated with MIR interpolation when only dlon/dlat is specified in the target 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.

It must give correct results to the target lat-lon grids earthkit-regrid offers. This is the current scope. When the geo library will be available this method will simply delegate the call to it and will offer a more generic solution.

@sandorkertesz
Copy link
Collaborator Author

sandorkertesz commented Mar 28, 2024

Refactored method and added more tests. The new name is to_latlon() and takes a gridspec as an input.

@sandorkertesz sandorkertesz changed the title Add generate_latlon method Add to_latlon method Mar 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.11%. Comparing base (d08b2ad) to head (5adc3a5).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop       #3      +/-   ##
===========================================
+ Coverage    96.17%   97.11%   +0.93%     
===========================================
  Files            5        6       +1     
  Lines          157      208      +51     
  Branches         9       14       +5     
===========================================
+ Hits           151      202      +51     
  Misses           3        3              
  Partials         3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HCookie
Copy link
Member

HCookie commented Sep 25, 2024

What's the current state of this branch?
Ideally, what needs to be worked on, tested, or improved?

@iainrussell
Copy link
Member

My new preferred name for this is to_latlons(). My problem with to_latlon() is that it's a term that we are used to meaning 'regrid to regular lat/lon'. to_latlons() sounds more explicit to me that what is returned will be multiple latitudes and multiple longitudes.

@sandorkertesz
Copy link
Collaborator Author

sandorkertesz commented Oct 3, 2024

Maybe it should be make_latlon() or make_latlons() since it is building them.

@pmaciel
Copy link
Member

pmaciel commented Oct 3, 2024

just to say I agree with everything here (to|make)_latlon(|s). I personally like make_latlons, for consistency as has been pointed out, but got used to to_latlon (so the exact opposite).

@iainrussell
Copy link
Member

Maybe it should be make_latlon() or make_latlons() since it is building them.

Well, the to_latlon() method in earthkit-data also generates lats and lons (through ecCodes, which generates them from the header information in the GRIB).

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.

5 participants