pangeo-data / xESMF

Universal Regridder for Geospatial Data
http://xesmf.readthedocs.io/
MIT License
183 stars 32 forks source link

Fix 80 #223

Open benjamin-cash opened 1 year ago

benjamin-cash commented 1 year ago

Description Modified specification of grids to allow for use of "pole_kind" specification available in ESMF to properly account for tripolar grids (e.g. MOM6).

Type of change New feature (non-breaking change which adds functionality)

How has this been tested? pytests on Frontera HPC used to confirm non-breaking status, comparison to output of previous method to confirm expected behavior of new method is being seen.

Checklist

My code follows the style guidelines of this project I have performed a self-review of my own code I have commented my code, particularly in hard-to-understand areas My changes generate no new warnings New and existing tests pass with my changes Any dependent changes have been merged and published

huard commented 1 year ago

@benjamin-cash @raphaeldussin

I noticed that the pole_kind option was introduced in ESMF 8.0.1, so added a bit of code to handle earlier versions.

From my perspective, the remaining open question is testing. The test data included in this PR weighs about 57M. I don't think it's a good idea to include such large test files in the main branch. Can we generate synthetic grids on the fly and test on those instead ?

benjamin-cash commented 1 year ago

Hi @huard - Yes, the size of the testing files worried me also. Does xesmf have the capability to generate a tripole grid on the fly? I noticed that there was a "mom6_like" grid being generated in the tests, but I didn't look to see if it was the same as the tripole. If it is then we should be able to drop the test files.

I notice that all of the build tests are failing - is this something I need to address? Looking briefly at the details it wasn't obvious to me what the issue was.

huard commented 1 year ago

I'll let Raphael answer the first question. For the second, I don't think the failures are due to anything you did, as it happens during the installation of dependencies. I'll try to find some help with this.

Zeitsperre commented 1 year ago

Build failures are due to a bug with setup-conda. Nothing we can do until the maintainers fix the action: https://github.com/conda-incubator/setup-miniconda/issues/274

Zeitsperre commented 1 year ago

Alternatively, this workaroudn should fix things if you'd like something working sooner: https://github.com/nextstrain/cli/commit/4a764976519ca5c540c745463548a9d883eae079

huard commented 1 year ago

Thanks !

benjamin-cash commented 1 year ago

I'll let Raphael answer the first question. For the second, I don't think the failures are due to anything you did, as it happens during the installation of dependencies. I'll try to find some help with this.

Any word on creating a tripole grid on the fly to test?

raphaeldussin commented 1 year ago

I think a good way to test would be to generate the bipole northern cap using a code derived from https://github.com/NOAA-GFDL/ocean_model_grid_generator/blob/2563e8e69e170a761fb409aab7addd6faf83cecf/ocean_grid_generator.py#L115

I don't have time to look at this right this moment but I can look into it next week

raphaeldussin commented 1 year ago

Hi @benjamin-cash

I just added a function to build a simple tripolar grid from scratch. That will allow us not to add large files into the repo. Unfortunately since you've committed large files into this branch, they will stay in the history even if you git rm them. The solution would be to create a new branch, manually copy the code changes and change the tests to use tripolar and regular grid created using the util functions so that we don't need to add files. To test the pole method, you can create a bogus field that is a function of the source lon and/or lat.

Let me know if you have any questions or need help with the new PR.

Thanks again for your contribution and I am really looking forward to adding this option!!

huard commented 1 year ago

I think git squash rewrites the history, so the large files would not be committed to the main branch. To be confirmed.

raphaeldussin commented 1 year ago

I don't master git (pun intended) enough to confirm/infirm. Good news is that there are not that many code changes and they could be copied to a new branch in 30 seconds with meld/vimdiff. The most time consuming part would be to rewrite the tests. @benjamin-cash let us know if you need help with this

benjamin-cash commented 1 year ago

Hi @raphaeldussin @huard - I've been on vacation the past week so am getting caught up today. Did anyone determine whether I can git rm the large files and not have them committed or if I need to create a new branch? Determining that answer is also beyond my git skillz.

raphaeldussin commented 1 year ago

@benjamin-cash hope you had a great vacation and welcome back. Personally I would just create a new branch and copy the code changes into it.

huard commented 1 year ago

The docs mention squash is a "rebase" operation, so I'm pretty confident squash and merge will not store the history with binary files.

https://www.git-tower.com/learn/git/faq/git-squash

huard commented 1 year ago

@benjamin-cash I did a rebase locally and pushed to a new PR. See #236. Please make sure I did not miss something in the operation.