Shorten initial lon lat routine

NoraLoose commented 1 month ago

Clean-up of `_make_initial_lon_lat_ds`

Remove optimization loop for better readabiliy; instead do only one iteration of Mercator projection (my hypothesis is that the optimization loop will only matter if both size_x and size_y are very large so that you approach the poles during (inverse) Mercator projection; let's double-check with Jeroen)
Fix bug in flipping coordinate procedure
More comments and more descriptive variable names

Clean-up of `_create_grid_ds`

Remove unnecessary inputs to this function
Add coordinates to xarray dataset

NoraLoose commented 1 month ago

Before merging I will check with Jeroen tomorrow if removing the optimization loop in the Mercator projection does indeed not matter for any but very large domains.

NoraLoose commented 1 month ago

After talking to Jeroen yesterday, I feel okay with removing the adhoc optimization loop (aka merging this PR), which according to Jeroen matters most of "intermediate"-sized domains such as the East Pacific. Here is a comparison of the matlab-generated and python-generated grid of Elizabeth's East Pacific domain (which may be a subset of their East Pacific domain), where the python-generated grid uses the code from this PR:

grid = Grid(
    nx=120,          # number of points in the x-direction (not including 2 boundary cells on either end)
    ny=160,          # number of points in the y-direction (not including 2 boundary cells on either end)
    size_x=3000,     # size of the domain in the x-direction (in km)
    size_y=4000,     # size of the domain in the y-direction (in km)
    center_lon=-126,  # longitude of the center of the domain
    center_lat=30,   # latitude of the center of the domain
    rot=32,          # rotation of the grid's x-direction from lines of constant longitude, with positive values being a counter-clockwise rotation
)

matlab_python_grid

There are slight differences in the longitudes and latitudes of matlab vs. python-generated grids (see last column) but I think that is okay, given that there are infinitely many ways to generate a grid with the given specifications.

Do you agree that we can merge this @TomNicholas?

TomNicholas commented 1 month ago

Thank you for looking into this so thoroughly @NoraLoose !

I'm a little bit confused why we would remove the loop at this point though. We now know what the loop is supposed to do, we've fixed the bug so that it actually does what it's supposed to do, and what it does is something useful (it makes the spacing more even in some cases). What's the remaining motivation to remove it?

NoraLoose commented 1 month ago

To me it is not clear that the loop makes the spacing more even. The loop computes the following multiplier mul, where the first two factors are the width and length of the center grid cell:

https://github.com/CWorthy-ocean/roms-tools/blob/9cee8a7195e07740df840065cfaa07ed5b884aa0/roms_tools/setup/grid.py#L261-L268

I'm not convinced that by comparing the center grid cells (which by construction are located on the equator at this point), you can infer anything about the overall evenness of the grid spacing.

Edit: After computing this multiplier, the domain width is scaled with this multiplier. That means that you modify the actual domain width that was specified by the user to something else.

TLDL: I do not completely understand what the loop is supposed to do. That's why I would rather keep it simple and remove it. Without the loop, a reasonable grid is generated that looks as good to me as with the loop.

TomNicholas commented 1 month ago

Okay if we don't fully understand it, its of questionable benefit, and the grid looks fine without it, then let's get rid of it! 😀

CWorthy-ocean / roms-tools