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

remapping weights script #45

Merged
merged 7 commits into from
Jan 23, 2025
Merged

remapping weights script #45

merged 7 commits into from
Jan 23, 2025

Conversation

anton-seaice
Copy link
Contributor

@anton-seaice anton-seaice commented Jan 16, 2025

Contributes to #44. The script makes remapping weights which map from all land cells into the nearest ocean cell and conserves the total volume. There is no spreading of runoff, or effort to put the runoff at the mouth of a bay/river when it is further away than the closest ocean.

Note:

There is not enough memory on the gadi login node to run this, its simplest to run in a terminal through are.nci.org.au

I get this warning, but it works ok:

WARNING: There was an error initializing an OpenFabrics device.

  Local host:   gadi-cpu-clx-1209
  Local device: mlx5_0

# Make a KDTree from the ocean cells
mask_tree = KDTree(mod_mesh_ds.centerCoords.isel(elementCount=mask_i))

# Using the KDTree, look up the nearest ocean cell to every destination grid cell in our weights file. Note our weights are indexed from 1 (i.e. Fortran style) but xarray starts from 0 (i.e. python style), so subract one from our destination grid cell indices.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, does "nearest" mean "smallest difference in grid index"? This may not correspond to smallest distance on the sphere, especially at the join between the eastern and western edges, and in the tripolar region.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

centerCoords is the geographic coordinate of the center of the cell. This function returns the index of the cell with the smallest difference in the geographic coordinate (by euclidean distance, not surface of a sphere). It won't wrap from east to western edge but should be ok for everywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, might need to take a look at whether anything fishy happens at the join - I hope there are no major rivers near 80E in the Indian or Arctic ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the haversine distance (with a small bug fix) successfully maps across the join - see #45 (review)

Copy link
Contributor

@aekiss aekiss left a comment

Choose a reason for hiding this comment

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

Thanks @anton-seaice, looks good. My only significant concern is whether this is nearest-neighbour on the sphere or on the grid.

@anton-seaice
Copy link
Contributor Author

Thanks @anton-seaice, looks good. My only significant concern is whether this is nearest-neighbour on the sphere or on the grid.

Its nearest-neighbour by euclidean distance between the center coordinates (in degrees) of the grid. This means that for the non-square cells, the results would be a bit weird (e.g. a nominal 0.25deg cell might be 6km in x and 11km in y in the Arctic, but be around 0.25 degrees in both directions).

@anton-seaice
Copy link
Contributor Author

@aekiss

It looks like using a Ball Tree instead of a KDTree allows for 'haversine' distances

See https://scikit-learn.org/stable/modules/neighbors.html#ball-tree

and
https://scikit-learn.org/stable/modules/generated/sklearn.metrics.pairwise.haversine_distances.html#sklearn.metrics.pairwise.haversine_distances

The haversine distances are appropriate for surface of the sphere, and should wrap east to west around the globe. Its a fairly simple change by the looks of it.

@aekiss
Copy link
Contributor

aekiss commented Jan 20, 2025

Cool, I don't think the difference between nearest-by-degrees and by-distance is likely to make much difference, but we may as well try use ball tree if it's easy.

@aekiss
Copy link
Contributor

aekiss commented Jan 20, 2025

Oh, I just re-read - if it also maps at the east-west join, then we should definitely use it. Will it also handle mapping across the tripole seam?

@anton-seaice
Copy link
Contributor Author

I made the update - the new scheme is slower to make weights but otherwise very similar (and still conserves runoff in the model)

Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

Thanks @anton-seaice!

I created a plot to convince myself that the BallTree query is working as expected and it revealed a small bug.

In the following, crosses shows every 50th cell centre. Blue crosses are land cells, yellow crosses are ocean cells. Red lines show which land cells are mapped to which ocean cells. Clearly there are land cells in the Antarctic being mapped to ocean cells in the Arctic.

Screenshot 2025-01-22 at 7 56 29 pm

As we worked out in our chat, this is because the coordinates need to be provided to the BallTree routines in the order (lat, lon), not (lon, lat). Switching this, as suggested in my review, fixes the issue.

Screenshot 2025-01-22 at 8 03 58 pm

mesh_generation/generate_rof_weights.py Outdated Show resolved Hide resolved
mesh_generation/generate_rof_weights.py Show resolved Hide resolved
mesh_generation/generate_rof_weights.py Show resolved Hide resolved
@anton-seaice
Copy link
Contributor Author

Thanks @dougiesquire - well spotted. I made the fix :)

image

Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

LGTM - nice work!

@anton-seaice anton-seaice merged commit 92cc161 into main Jan 23, 2025
4 checks passed
@anton-seaice anton-seaice deleted the 44-runoff-remapp branch January 23, 2025 03:19
@aekiss
Copy link
Contributor

aekiss commented Jan 23, 2025

Great work both, LGTM!

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.

3 participants