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

Allow regridding for projections in non-degree type units #178

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

stephenworsley
Copy link
Contributor

@stephenworsley stephenworsley commented May 4, 2022

Addresses #222

Some projections, like OSGB, have units which cannot be converted to degrees (i.e. meters) and cause regridding to fail. This PR assumes that where a coordinate system exists, units ought to already be compatible with it.

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #178 (432ebc2) into main (97d0257) will increase coverage by 0.01%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   98.73%   98.75%   +0.01%     
==========================================
  Files          33       33              
  Lines        3646     3680      +34     
==========================================
+ Hits         3600     3634      +34     
  Misses         46       46              
Files Coverage Δ
esmf_regrid/schemes.py 95.60% <100.00%> (+0.03%) ⬆️
esmf_regrid/tests/unit/schemes/__init__.py 100.00% <100.00%> (ø)
...regrid/tests/unit/schemes/test_ESMFAreaWeighted.py 100.00% <100.00%> (ø)
...smf_regrid/tests/unit/schemes/test_ESMFBilinear.py 100.00% <100.00%> (ø)
esmf_regrid/tests/unit/schemes/test_ESMFNearest.py 100.00% <100.00%> (ø)
...egrid/tests/unit/schemes/test__cube_to_GridInfo.py 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@trexfeathers
Copy link
Contributor

Using pyproj.Proj could be an easy way of converting these cases to degrees.

@stephenworsley
Copy link
Contributor Author

Using pyproj.Proj could be an easy way of converting these cases to degrees.

This conversion ought to happen at this point:
https://github.com/SciTools-incubator/iris-esmf-regrid/blob/eca3a2a0dc4d5a2e8a2c8ae8535127f719d5fa4c/esmf_regrid/_esmf_sdo.py#L224-L225

I would imagine that cartopy would be calling Proj here anyway. I think that in the case where units are supplied in meters, at this point cartopy describes the projection as being described in meters and is expecting data to be in meters. There is still a case that there could be more to be done to ensure that units are compatible with the coordinate system, but I'm not sure what it would mean for a coordinate to have a unit which disagreed with the coordinate system.

The approach I've gone for is to treat the coordinate system as the single source of truth for a coordinate where it exists. This seems the simplest way to do things for the time being.

@lbdreyer
Copy link
Member

lbdreyer commented May 5, 2022

Do you plan to add some testing to this PR?

@stephenworsley
Copy link
Contributor Author

stephenworsley commented May 5, 2022

Do you plan to add some testing to this PR?

Yes, I have been thinking about adding tests. I think I would ideally want some OSGB data (or a subset of such data) to work with, since thats where this problem showed up. The closest I can find in iris-test-data seems to be transverse mercator (which I think OSGB is an instance of). If you have any other ideas of data to test this on let me know.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

@SciTools-incubator/esmf-regrid-devs This pull-request is stale due to a lack of activity in the last 90 days. Remove stale label or comment, otherwise this pull-request will close automatically in 7 days time.

@github-actions github-actions bot added the Stale: Closure warning This stale issue or pull-request has been marked for closure label Aug 4, 2022
@github-actions
Copy link
Contributor

@Scit@SciTools-incubator/esmf-regrid-devs This stale pull-request has been automatically closed due to no community activity

@github-actions github-actions bot added the Stale: Closed This stale issue or pull-request has been closed due to no activity label Aug 12, 2022
@github-actions github-actions bot closed this Aug 12, 2022
@stephenworsley stephenworsley removed Stale: Closure warning This stale issue or pull-request has been marked for closure Stale: Closed This stale issue or pull-request has been closed due to no activity labels Aug 31, 2023
@stephenworsley
Copy link
Contributor Author

I think this is still worth getting done

@trexfeathers trexfeathers self-assigned this Oct 11, 2023
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Thanks @stephenworsley, a few comments for you

esmf_regrid/schemes.py Outdated Show resolved Hide resolved
standard_names=["projection_x_coordinate", "projection_y_coordinate"],
units="m",
)
data = np.arange(n_lats_src * n_lons_src).reshape([n_lats_src, n_lons_src])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR: we do this so often I'm starting to wonder whether it's worth having a convenience for it.

esmf_regrid/tests/unit/schemes/__init__.py Outdated Show resolved Hide resolved
esmf_regrid/tests/unit/schemes/__init__.py Outdated Show resolved Hide resolved
esmf_regrid/tests/unit/schemes/test_ESMFAreaWeighted.py Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Oct 26, 2023

CLA assistant check
All committers have signed the CLA.

@trexfeathers
Copy link
Contributor

Thanks @stephenworsley!

@trexfeathers trexfeathers merged commit 4f685cf into SciTools:main Nov 1, 2023
15 checks passed
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.

4 participants