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

Fix 80 #223

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Fix 80 #223

wants to merge 12 commits into from

Conversation

benjamin-cash
Copy link

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
Copy link
Contributor

huard commented Jan 16, 2023

@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
Copy link
Author

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
Copy link
Contributor

huard commented Jan 16, 2023

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
Copy link
Contributor

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

@Zeitsperre
Copy link
Contributor

Alternatively, this workaroudn should fix things if you'd like something working sooner: nextstrain/cli@4a76497

@huard
Copy link
Contributor

huard commented Jan 17, 2023

Thanks !

@benjamin-cash
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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!!

@raphaeldussin raphaeldussin self-requested a review January 27, 2023 16:05
Copy link
Contributor

@raphaeldussin raphaeldussin left a comment

Choose a reason for hiding this comment

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

this branch contains large files and needs to be replaced by a new one without files and using util functions to create grids

@huard
Copy link
Contributor

huard commented Jan 27, 2023

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

@raphaeldussin
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Contributor

huard commented Feb 3, 2023

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
Copy link
Contributor

huard commented Feb 3, 2023

@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.

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.

None yet

4 participants