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

Add a function to get the full-size of xg and yg from input files #195

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

Conversation

cspencerjones
Copy link
Contributor

This function gets XG and YG from .mitgrid files. They are the full size (i.e. they include the points on the right hand side of the final gridpoint in both x and y directions. This is useful if you want to determine which tile and individual lat/lon point is on.

@raphaeldussin
Copy link
Contributor

what was wrong with get_grid_from_input ?

def get_grid_from_input(gridfile, nx=None, ny=None, geometry='llc',

@cspencerjones
Copy link
Contributor Author

cspencerjones commented Mar 12, 2020

It doesn't do what I want - it doesn't get the rightmost gridpoints.

Also it doesn't tile in the way that I want

Copy link
Member

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @cspencerjones. I appreciate that the existing get_grid_from_inputs function did not work for the reasons you stated. However, I would strongly prefer that we modify that function to meet your needs, rather than introduce a new one with much of the same code.

I see two separate features here:

  • The ability to get the extra "outer" grid point from the mitgrid files. This could be an optional flag to get_grid_from_inputs, maybe outer=False as a default, to keep backwards compatibility
  • The ability to reshape a dataset in tiles of a specified shape.

Could we do these as two separate functions, or is there some fundamental reason why they need to be in a single function?

blankList=bl)
# test types
assert type(ds) == xarray.Dataset
assert type(ds['XG']) == xarray.core.dataarray.DataArray
Copy link
Member

Choose a reason for hiding this comment

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

use isinstance here

@rabernat
Copy link
Member

rabernat commented Apr 7, 2020

@cspencerjones - are you planning to continue to work on this?

@cspencerjones
Copy link
Contributor Author

It's not my priority. If we think that these changes would be useful for other people then I can but I'm not sure that there is actually need for them right now. So probably no, I will just keep using my own branch. Do you have a strong opinion?

@AaronDavidSchneider
Copy link
Contributor

Hi @cspencerjones,
I could really use the outer grid points (xESMF needs outer grid points for conservative regridding). I would be happy if this PR could be somehow resumed :D I'd be happy to help.

@cspencerjones
Copy link
Contributor Author

@AaronDavidSchneider I do not have time to work on this at the moment, since my project has moved away from this problem, at least temporarily. The code is quite dense and hard to follow, and as @rabernat mentions above my new code actually repeats a lot of what's in the original function. However you are welcome to take it and see how it works for you. If it does, then maybe you could contribute to this pull request as well?

@rabernat
Copy link
Member

rabernat commented Jan 6, 2021

Yes it would be great if @AaronDavidSchneider would be able to pick up this PR! We will be happy to help you.

From a technical standpoint, it is probably easiest for you to check out @cspencerjones's branch and make a new PR from your own fork.

@AaronDavidSchneider
Copy link
Contributor

From a technical standpoint, it is probably easiest for you to check out @cspencerjones's branch and make a new PR from your own fork.

That sounds good. I will try my best to get it working. I am not quite sure when I will have the time for it.

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