Skip to content

fix few issues with byc #56

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix few issues with byc #56

wants to merge 4 commits into from

Conversation

cosunae
Copy link
Contributor

@cosunae cosunae commented Feb 2, 2025

Purpose

Attempt to fix the interpolation using byc.

Remove the sections below which do not apply.
Not yet ready, the test for icon-ch2 is failing.

Checklist

Before submitting this PR, please make sure:

  • You have followed the coding standards guidelines established at Code Review Checklist.
  • Docstrings and type hints are added to new and updated routines, as appropriate
  • All relevant documentation has been updated or added (e.g. README)
  • Unit tests are added or updated for non-operator code
  • New operators are properly tested

Additionally, if the PR updates the version of the package

  • The new version is properly set
  • A Tag will be created after the bump

Review

For the review process follow the guidelines at Checklist

@cosunae cosunae marked this pull request as draft February 2, 2025 23:22

xy = np.array(points_src).T
uv = np.array(points_dst).T

indices, weights = _linear_weights_cropped_domain(xy, uv)

return _icon2regular(field, dst, indices, weights)
return _icon2regular(field, dst, indices, weights).assign_coords(
lon=(("y", "x"), gx), lat=(("y", "x"), gy)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be lon or longitude?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this only applies if the dst grid is defined in geolatlon CRS

@@ -485,11 +486,13 @@ def iconremap(

gx, gy = np.meshgrid(dst.x, dst.y)
transformer_dst = Transformer.from_crs(dst.crs.wkt, utm_crs)
points_dst = transformer_dst.transform(gx.flat, gy.flat)
points_dst = transformer_dst.transform(gy.flat, gx.flat)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which coordinate system are you using for your test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

geolatlon

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, I would rather add always_xy to the previous line because this change is breaking support for all CRSs that are in the traditional order

@@ -35,9 +35,6 @@ def speed(u: xr.DataArray, v: xr.DataArray) -> xr.DataArray:
the horizontal wind speed [m/s].

"""
if u.origin_x != 0.0 or v.origin_y != 0.0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we transition towards ICON native grid support and or other grids, I think we should decommission the C staggering (i.e. origin_x, origin_y). Still variables will be staggered in z

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless we drop support for COSMO data from the archive, this will need to live on. The meaning of the origin_x and origin_y attributes need to be defined in the context of the ICON native grid. Then, we can adapt the checks.

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