Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Structured grids barycentric coordinates - #2037

Merged
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp
Jul 1, 2025
Merged

Structured grids barycentric coordinates#2037
VeckoTheGecko merged 24 commits into
v4-devfrom
xgrid-interp

Conversation

@VeckoTheGecko

@VeckoTheGeckoVeckoTheGecko commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Changes:

  • Require dataset to have lon lat grid of F points (and validates that this is the right format)
  • Index search and barycentric coordinate calculation on these F-points (both for 1D and 2D lon and lat)
    • Assumes that depth is 1D
    • Uses algorithm from v3 (in future we're sure this can be optimised to be more robust and faster - but this is good for now)
  • ravel and unravel method

Here is a diagram to help visualise what's happening here:

image

Diagram on NEMO/MITgcm indexing in general

image

Note that result from the search() method is wrt. the red points (i.e., yi, xi = 0, 0, eta, xsi = 0.5, 0.5 means the particle is in the cell centre with lower left being red point at (0, 0).

One thing to note: Particles on the edge of the model grid are not representable in the F-points (in the diagram, this is evident in the NEMO model for the lower left edge of the grid). In the periodic case, the grid has a halo anyway so this is a non-issue. The grid is not patched to be a "full grid" on initialisation - we instead work close to the original dataset.

If we have a particle in tracer cell (yi, xi) = 0,0, then the relevant velocities for interpolation are:

  • NEMO:
    • U[y, x] -> U[1,0] and U[1,1]
    • V[y, x] -> V[0,1] and V[1,1]
  • MITgcm
    • U[y, x] -> U[0,0] and U[0,1]
    • V[y, x] -> V[0,0] and V[1,0]

The lining up of these indices with on-disk representation will be handled in the interpolator itself (in a future PR).

@VeckoTheGecko
VeckoTheGecko marked this pull request as draft June 13, 2025 14:40
@VeckoTheGecko
VeckoTheGecko marked this pull request as ready for review June 24, 2025 09:34
@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

@erikvansebille I'm not sure what the code at da87f9b does (in particular the if grid.mesh == 'spherical': block), something to do with the international dateline? Could you provide some insight here?

Also insight on _reconnect_bnd_indices would be helpful (first mention: 812be0c)

Happy to discuss irl, and will update here accordingly.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

This error makes sense, the current implementation is:

 ...
da=xr.DataArray(
data=np.full((1, 1, 1, 1), value),
dims=["time", "ZG", "YG", "XG"],
coords={
"ZG": (["ZG"], np.arange(1), {"axis": "Z"}),
"YG": (["YG"], np.arange(1), {"axis": "Y"}),
"XG": (["XG"], np.arange(1), {"axis": "X"}),
"lon": (["XG"], np.arange(1), {"axis": "X"}),
"lat": (["YG"], np.arange(1), {"axis": "Y"}),
"depth": (["ZG"], np.arange(1), {"axis": "Z"}),
"time": (["time"], np.arange(1), {"axis": "T"}),
},
)
breakpoint()
grid=XGrid(xgcm.Grid(da))
...

Basically making a 1 point structured grid and then going forward with that.

I think this implementation should change (it doesn't make sense for a constant field to have its own grid, especially now since grids now have the concepts of searching, raveling, and "out of bounds"). Not sure exactly how yet, having a think...

@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

@VeckoTheGecko - at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

@VeckoTheGecko

VeckoTheGecko commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

Comment threadparcels/_index_search.py
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py
@fluidnumericsJoe

fluidnumericsJoe commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

at a minimum, a single point grid would be a single tracer point, which would give 4 vorticity points that form the boundary of the tracer cell. If you have only one vorticity point, there are no tracer cells.

Yes, that would get past the initialiser - but would fail on eval due to the index searching etc.. I'm also thinking about futureproofing (does this mean for a constant Field that an extra ei for each particle causing a n_particles * 8 byte memory overhead?). Going a different route avoids this memory overhead. I might just xfail for this PR and fix in a different one

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

@erikvansebille

Copy link
Copy Markdown
Member

I don't understand why it fails on index searching. An Arakawa-C grid is not completely defined in the example you've provided with just an XG,YG point. It's also not completely defined with just an XC,YC point; both sets of points are needed to define a grid. A single finite volume cell is defined completely by specifying the four corner points of the volume (the four XG,YG F-points) and the cell center (which can be inferred as the average of the four corner points - this is the XC,YC point).

Not sure if this is super-pertinent to this discussion, but note that in Parcels v3, we use 'one-dimensional fields' to represent e.g. horizontally uniform diffusivity or variables that would only depend on depth. See for an example e.g.
https://github.com/OceanParcels/Parcels/blob/ae2d508fa8608ab81f3b5e9e1c34acc220b2c1eb/docs/examples/example_brownian.py#L26-L30
These fields have only one longitude and latitude point, and by definition cannot throw an OutOfBounds error.

It's quite useful (albeit a bit hacky?) to keep support for these one-dimensional fields?

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe full traceback

Details

============================= test session starts ==============================
platform darwin -- Python 3.12.10, pytest-8.3.5, pluggy-1.5.0 -- /Users/Hodgs004/miniforge3/envs/parcels-dev/bin/python3.12
cachedir: .pytest_cache
metadata: {'Python': '3.12.10', 'Platform': 'macOS-15.3.1-arm64-arm-64bit', 'Packages': {'pytest': '8.3.5', 'pluggy': '1.5.0'}, 'Plugins': {'anyio': '4.9.0', 'html': '4.1.1', 'metadata': '3.1.1', 'hypothesis': '6.131.9', 'nbval': '0.11.0', 'reportlog': '0.1.2'}}
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/Users/Hodgs004/coding/repos/parcels/.hypothesis/examples'))
rootdir: /Users/Hodgs004/coding/repos/parcels
configfile: pyproject.toml
plugins: anyio-4.9.0, html-4.1.1, metadata-3.1.1, hypothesis-6.131.9, nbval-0.11.0, reportlog-0.1.2
collecting ... collected 1 item
tests/v4/test_fieldset.py::test_fieldset_add_constant_field FAILED [100%]
=================================== FAILURES ===================================
_______________________ test_fieldset_add_constant_field _______________________
fieldset = <parcels.fieldset.FieldSet object at 0x1771c0980>
def test_fieldset_add_constant_field(fieldset):
> fieldset.add_constant_field("test_constant_field", 1.0)
tests/v4/test_fieldset.py:42: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ parcels/fieldset.py:177: in add_constant_field
grid = XGrid(xgcm.Grid(da))
parcels/xgrid.py:55: in __init__
assert_valid_lat_lon(ds["lat"], ds["lon"], grid.axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ da_lat = <xarray.DataArray 'lat' (YG: 1)> Size: 8B
array([0])
Coordinates:
* YG (YG) int64 8B 0
lat (YG) int64 8B 0
Attributes:
axis: Y
da_lon = <xarray.DataArray 'lon' (XG: 1)> Size: 8B
array([0])
Coordinates:
* XG (XG) int64 8B 0
lon (XG) int64 8B 0
Attributes:
axis: X
axes = OrderedDict({'Y': <parcels.Axis 'Y' (periodic, boundary=None)>
Axis Coordinates:
* center YG, 'T': <parcels.Axis '...xis Coordinates:
* center XG, 'Z': <parcels.Axis 'Z' (periodic, boundary=None)>
Axis Coordinates:
* center ZG})
def assert_valid_lat_lon(da_lat, da_lon, axes: _XGCM_AXES):
"""
Asserts that the provided longitude and latitude DataArrays are defined appropriately
on the F points to match the internal representation in Parcels.
- Longitude and latitude must be 1D or 2D (both must have the same dimensionality)
- Both are defined on the left points (i.e., not the centers)
- If 1D:
- Longitude is associated with the X axis
- Latitude is associated with the Y axis
- If 2D:
- Lon and lat are defined on the same dimensions
- Lon and lat are transposed such they're Y, X
"""
assert_all_dimensions_correspond_with_axis(da_lon, axes)
assert_all_dimensions_correspond_with_axis(da_lat, axes)
dim_to_position = {dim: get_position_from_dim_name(axes, dim) for dim in da_lon.dims}
dim_to_position.update({dim: get_position_from_dim_name(axes, dim) for dim in da_lat.dims})
for dim in da_lon.dims:
if get_position_from_dim_name(axes, dim) == "center":
> raise ValueError(
f"Longitude DataArray {da_lon.name!r} with dims {da_lon.dims} is defined on the center of the grid, but must be defined on the F points."
)
E ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
parcels/xgrid.py:328: ValueError
=============================== warnings summary ===============================
../../../miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7
/Users/Hodgs004/miniforge3/envs/parcels-dev/lib/python3.12/site-packages/geopandas/_compat.py:7: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
import shapely.geos
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/tests/v4/test_fieldset.py:21: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(ds))
tests/v4/test_fieldset.py::test_fieldset_add_constant_field
/Users/Hodgs004/coding/repos/parcels/parcels/fieldset.py:177: DeprecationWarning: The `periodic` argument will be deprecated. To preserve previous behavior supply `boundary = 'periodic'.
grid = XGrid(xgcm.Grid(da))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/v4/test_fieldset.py::test_fieldset_add_constant_field - ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.
======================== 1 failed, 3 warnings in 4.26s =========================

@VeckoTheGecko
VeckoTheGeckoforce-pushed the xgrid-interp branch 2 times, most recently from 7cc9e02 to 3f453b3CompareJune 25, 2025 13:35
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Let's postpone this a couple days.... Working on a proposal that affects this

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

I've just rebased this and updated the API to line up with #2048 - ready for review.

@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

@fluidnumerics-joe getting some failing tests now with FieldSet.add_constant_field: ValueError: Longitude DataArray 'lon' with dims ('XG',) is defined on the center of the grid, but must be defined on the F points.

Resolved with #2058 - all v4 tests are passing in that PR

Comment threadparcels/_index_search.py
Comment threadparcels/_index_search.py Outdated
yi -= 1
elif eta > 1 + tol:
yi += 1
(yi, xi) = _reconnect_bnd_indices(yi, xi, grid.ydim, grid.xdim, grid.mesh)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed if we don't do periodic grids anymore?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I was wondering if this wasn't for periodic grids, but instead was to do with tripolar grids - would that make sense? I thought the edges/wrapping in that grid would result in the index search needing to be "reconnected" to the grid like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, you may indeed be right. Let's leave in for now and we can see whether this is still necessary when we test curvilinear NEMO grid in the Arctic

Comment threadparcels/basegrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py Outdated
Comment threadparcels/xgrid.py
Comment threadtests/v4/test_xgrid.py
@VeckoTheGecko

Copy link
Copy Markdown
ContributorAuthor

Just implemented the review changes. Still a couple pending items above that we didn't discuss during our meeting @erikvansebille - after that I think we're good to merge.

@erikvansebille

Copy link
Copy Markdown
Member

Good to merge, as far as I'm concerned

@VeckoTheGecko
VeckoTheGecko merged commit 7baf916 into v4-devJul 1, 2025
@VeckoTheGecko
VeckoTheGecko deleted the xgrid-interp branch July 1, 2025 12:52
@github-project-automationgithub-project-automationBot moved this from Backlog to Done in Parcels developmentJul 1, 2025
@VeckoTheGeckoVeckoTheGecko mentioned this pull request Jul 11, 2025
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@VeckoTheGecko@fluidnumericsJoe@erikvansebille