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

Update regridder.grid property to ensure cf compliant coordinates #736

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Feb 10, 2025

Description

This pull request introduces a new feature to ensure CF-compliance of coordinate variables in the regridder module. xESMF requires coordinates to be CF-compliant in order to map to them using CF-xarray.

Key changes include:

New Feature Implementation:

  • xcdat/regridder/accessor.py: Added the _ensure_cf_compliance method to ensure coordinate variables have the "axis" and "standard_name" attributes required for CF-compliance. This method is now called within _get_axis_data.

Test Enhancements:

  • tests/test_regrid.py: Added a new test test_grid_adds_cf_attributes_to_non_cf_compliant_coords to verify that the regridder correctly adds CF attributes to non-compliant coordinates.

Import Adjustments:

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

- CF-compliance is required for xESMF to properly interpret coordinates using CF-xarray
@tomvothecoder tomvothecoder added the type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. label Feb 10, 2025
@tomvothecoder tomvothecoder self-assigned this Feb 10, 2025
@tomvothecoder tomvothecoder changed the title Update grid property to ensure cf compliant coordinates Update regridder.grid property to ensure cf compliant coordinates Feb 10, 2025
Copy link
Collaborator Author

@tomvothecoder tomvothecoder left a comment

Choose a reason for hiding this comment

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

Hey @pochedls and @jasonb5, this PR fixes #718. Can you test and review?

I'd like to get this merged in the next day or two.

@tomvothecoder tomvothecoder added the type: enhancement New enhancement request label Feb 10, 2025
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8824b32) to head (69896f1).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #736   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1621      1630    +9     
=========================================
+ Hits          1621      1630    +9     

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

@jasonb5
Copy link
Collaborator

jasonb5 commented Feb 12, 2025

Hey @pochedls and @jasonb5, this PR fixes #718. Can you test and review?

I'd like to get this merged in the next day or two.

I will review this PR today.

@pochedls
Copy link
Collaborator

pochedls commented Feb 12, 2025

Things are working now, but I think there is still an issue. Take a look at the map from xcdat (this PR):

Screenshot 2025-02-12 at 11 40 16 AM

compared to calling xesmf directly:

Screenshot 2025-02-12 at 11 41 10 AM

A vague idea/possibility: In curvilinear grids lat/lon are often represented as an index (e.g., [0, 1, 2, ...]) for latitude["lat", "lon"] and longitude["lat", "lon"]. I'm wondering if the regridder is interpreting lat (the index) as the source grid points instead of the actual latitude values that are a function of lat/lon. [I might have lat/latitude reversed in this example, but something like this may be going on].

@tomvothecoder
Copy link
Collaborator Author

@pochedls Thanks Steve. It looks like we need more work on this. I'll push back the release of v0.8.0 depending on when the next E3SM Unified is released (which is probably being pushed back from 2/15)

A vague idea/possibility: In curvilinear grids lat/lon are often represented as an index (e.g., [0, 1, 2, ...]) for latitude["lat", "lon"] and longitude["lat", "lon"]. I'm wondering if the regridder is interpreting lat (the index) as the source grid points instead of the actual latitude values that are a function of lat/lon. [I might have lat/latitude reversed in this example, but something like this may be going on].

Hmm this is a good question. I'll step through the code to see which values are being used for latitude.

@tomvothecoder
Copy link
Collaborator Author

A vague idea/possibility: In curvilinear grids lat/lon are often represented as an index (e.g., [0, 1, 2, ...]) for latitude["lat", "lon"] and longitude["lat", "lon"]. I'm wondering if the regridder is interpreting lat (the index) as the source grid points instead of the actual latitude values that are a function of lat/lon. [I might have lat/latitude reversed in this example, but something like this may be going on].

I explain the issue here. You're correct, nlat and nlon are being used to represent the grid because they are considered the index/dimension coordinates. However, these coordinates contain integers representing the indices of the coordinates. lat and lon are ancillary coordinates and contain the actual values that are needed for regridding with xESMF.

This is mainly a metadata/coordinates issue rather than an issue in xCDAT's logic. We'll need to think more carefully about how we can retrieve lat and lon since they are ancillary coordinates. The regridder.grid property calls _get_axis_data() which then calls xc.get_dim_coords() (only gets dimension coordinates).

Copy link
Collaborator

@jasonb5 jasonb5 left a comment

Choose a reason for hiding this comment

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

@tomvothecoder
Copy link
Collaborator Author

More work needs to be done based on @jasonb5's comment here: #718 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. type: enhancement New enhancement request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[Bug]: Getting data CF-compliant for regridding (with xesmf)
3 participants