Skip to content

Vertical Coordinate Interpolators - #3037

Open
wyatt-fluidnumerics wants to merge 20 commits into
mainfrom
vertical-coordinate-interpolators
Open

wyatt-fluidnumerics wants to merge 20 commits into
mainfrom
vertical-coordinate-interpolators

Conversation

@wyatt-fluidnumerics

@wyatt-fluidnumerics wyatt-fluidnumerics commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Description

This PR addresses the need for more complex interpolation methods for time varying z dimensions, while maintaining flexibility for users and alignment with the current interpolator API.

The key change is the introduction of a VerticalCoordinateInterpolator abstract class that sits at the same level as the Vector and Scalar interpolator classes. At its core, the VerticalCoordinateInterpolator class enables any existing or user madeScalarInterpolator to be used to interpolate the vertical coordinate. This is done through its base method _columns_from_scalar, which takes in a scalar interpolator and uses it to run a horizontal interpolation on every vertical interface at each particle's horizontal position. These interfaces are then used to reconstruct the vertical columns at each particle position, which can be used for the vertical grid search.

For time varying z coordinate datasets, a vertical coordinate interpolator is set during Fieldset creation, and can be overwritten by the user.

This PR also adjusts all UxInterpolators to perform linear in time interpolation for data that falls between two model snapshots. This is required to avoid large temporal jump discontinuities in time varying vertical grids.

Checklist

  • Tests added
  • This PR targets the correct branch (main for normal development, v3-support for v3 support)

AI Disclosure

  • This PR contains AI-generated content.
    • I have tested any AI-generated content in my PR.
    • I take responsibility for any AI-generated content in my PR.
    • Describe how you used it (e.g., by pasting your prompt): I guided claude opus 5.5 through the implementation. I have thoroughly reviewed all code, and made edits where needed.

@wyatt-fluidnumerics
wyatt-fluidnumerics marked this pull request as ready for review November 10, 2026 20:33

@erikvansebille erikvansebille left a comment

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.

Looks good! Some comments below

Comment thread src/parcels/_core/particleset.py Resolved
Comment thread src/parcels/_core/uxgrid.py Outdated
Comment on lines +78 to +86
warnings.warn(
"Time-varying (3D) z coordinates are experimental and may cause significant memory overhead that "
f"leads to OOM errors. This z coordinate has sizes {dict(z.sizes)} ({z.nbytes / 1e9:.3g} GB). "
f"Assumptions: z has dims {z.dims}, i.e. it is defined at the layer interfaces ('zf') on the mesh "
"nodes or faces, and is strictly increasing along 'zf'; each particle's z column is interpolated with "
"the grid's `z_interp_method` (by default barycentrically from its face's nodes for node z, or "
"constant over its face for face z), and linearly in time between z snapshots.",
FieldSetWarning,
stacklevel=5,

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.

But I thought this warning may not be necessary anymore because the 4D depth fields can use the ChunkCachedArray or WindowedArray backend?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is true, I had kept it because we currently don't set it to be ChunkCachedArray or WindowedArray explicitly, but we should probably do this in _init_time_varying_z and then remove the warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in cb13494

f"A 3D z must have dims ('time', 'zf', 'n_node') or ('time', 'zf', 'n_face'), got {z.dims}"
)

if z.dims[-1] == "n_node":

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.

Do we also need to check that the dimension order of z is such that the last is n_node or n_face? Or is that done somewhere else?

@wyatt-fluidnumerics wyatt-fluidnumerics Nov 13, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The check above this line verifies the dimension ordering and throws an error if the dimension ordering doesn't have n_node/n_face last. That being said, in retrospect, and comparing to the XGrid side, I don't think we should error here, and instead add a function similar to preprocess_sgrid_model_data, which would rearrange the dims to guarantee this order but allow us to ingest data of any order.

I'll add that to this PR.

isinstance(value, XVerticalCoordinateConstant)
and get_dim_position(self.sgrid_metadata, depth_dims[-1]) != "face"
):
raise ValueError(f"XVerticalCoordinateConstant needs depth on the cell centres, got dims {depth_dims}")

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.

Can we be more explicit here what the cell centres are called in this dataset? Would we know that?


if "Z" in self.axes and ds.depth.ndim == 4:
zi, zeta = self._search_time_varying_depth(z, yi, xi, ti, tau)
zi, zeta = self._search_time_varying_depth(z, yi, eta, xi, xsi, ti, tau)

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.

I only realise this now, but would it not make more sense to use the order ti, tau, yi, eta, xi, xsi for the position here - in order to harmonise dimensionality throughout the codebase?

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.

I can't comment on individual lines, so below some comments on the notebook

  • rename the title to use lowercase `time varying'?
  • use the hide-input tag for the dataset creation cell, so that users don't get overwhelmed by it? It's not that relevant for most users how the dataset is created?
  • Type-o in coordiante
  • Is it expected that the particles don't move horizontally on the right panel of the last animation?

self._ds = model_data
self.mesh = mesh
self._spatialhash = None
self._z_interp_method = None

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.

The _interp_method is set on the Field. Why is the _z_interp_method then set on the grid? Can different Fields on the same grid not have different interpolation methods?

@wyatt-fluidnumerics wyatt-fluidnumerics Nov 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess in my mind the time varying z coordinate is still a product of the grid. If interpolation between variables is to be as physically consistent as possible I don't think different fields on the same grid should use different interpolators for z, since it would mean that different fields think a particle is at different positions in the water column. That would be a fundamentally different thing when compared to using different interpolators for different fields that all receive the same particle position from the search.

Additionally from a structural point of view the z_interp_method is used during particle search which is grid functionality rather than field functionality, and it is not clear to me if there is a clean way to expose field specific z interpolators to the particle search.

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.

Good point that these interpolators are essentially for the searching of the particle locations in the grid - not for interpolating the values. So you're right that these belong on the grid.

But perhaps we should make clearer then (in the naming of the functions? in the docstrings?) that these z_interp_methods are indeed fundamentally different from 'normal' interpolators

self._ds = model_data
self.mesh = mesh
self._spatialhash = None
self._z_interp_method = None

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.

Should we now rename interp_method to h_interp_method or something similar to make explicit that these are horizontal interpolation methods? Or should/could that be done in another pR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Potentially, though my hope is that anyone who is looking into the internals enough to run into this will also run into the doc string on the VerticalCoordinateInterpolator which states this explicitly. Maybe we try like this and if the naming becomes a problem we change it in another PR?

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.

Given the comment above, I now think that we shouldn't rename the other interpolators, since they do something fundamentally different (interpolating values onto a location, vs finding a location in a grid)

Comment on lines +55 to +56
def __repr__(self):
return f"{self.__class__.__name__}(...)"

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.

Should we move this interpolators.__repr__ to the repr_utils.py file, so they are all in one place

Comment on lines +670 to +674
upper_half = bcoord >= 0.5
lower_face = np.where(upper_half, face_index, face_index - 1)
centre_bcoord = np.where(upper_half, bcoord - 0.5, bcoord + 0.5)
below_first_centre = lower_face < 0
return np.where(below_first_centre, 0, lower_face), np.where(below_first_centre, 0.0, centre_bcoord)

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.

Should we use the offsets here? Because some models are indexed by the upper-right corner and others by the lower-left corner. Or is that not relevant here?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants