Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions test/core/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,21 @@ def test_open_dataset(gridpath, datasetpath, mesh_constants):
nt.assert_equal(len(uxds_var2_ne30.uxgrid._ds.data_vars), mesh_constants['DATAVARS_outCSne30'])
nt.assert_equal(uxds_var2_ne30.source_datasets, str(data_path))


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.

We need another test case to make sure the single file has proper grid definitions in it (i.e. If not throw exception)

def test_open_dataset_single_combined_mpas_file(gridpath):
"""Loads a combined MPAS grid-and-data file with a single argument."""

file_path = gridpath("mpas", "QU", "mesh.QU.1920km.151026.nc")

uxds_single = ux.open_dataset(file_path)
uxds_pair = ux.open_dataset(file_path, file_path)

nt.assert_equal(uxds_single.uxgrid.source_grid_spec, "MPAS")
nt.assert_equal(uxds_single.source_datasets, str(file_path))
nt.assert_equal(uxds_single.sizes["n_face"], uxds_pair.sizes["n_face"])
nt.assert_equal(set(uxds_single.data_vars), set(uxds_pair.data_vars))


def test_open_mf_dataset(gridpath, datasetpath, mesh_constants):
"""Loads multiple datasets with their grid topology file using
uxarray's open_dataset call."""
Expand Down
20 changes: 17 additions & 3 deletions uxarray/core/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ def list_grid_names(

def open_dataset(
grid_filename_or_obj: str | os.PathLike[Any] | dict | Dataset,
filename_or_obj: str | os.PathLike[Any],
filename_or_obj: str | os.PathLike[Any] | None = 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.

While technically there is nothing wrong with this, I'd like to share a few thoughts on use-case logic:

  1. Defaulting filename_or_obj to None might not be the best idea since, in most cases, it is needed, i.e. single file dataset opening is anticipated to occur much less.
  2. Also, more of a terminology thing, but my observation is that when there is only file that has the grid information in it as well, it is referred to as the data or diagnostics file rather than the grid file. That said, I'd recommend to handle the single file case when grid_filename_or_obj is given None, and filename_or_obj has both grid definition and data in it.

Copy link
Copy Markdown
Contributor Author

@rajeeja rajeeja Apr 2, 2026

Choose a reason for hiding this comment

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

I went with ux.open_dataset(file) as the smallest backward-compatible public API change and avoids open_dataset(None, file) calling pattern. I agree the naming is a little awkward, but my intent was to add the one-file shorthand without changing the existing two-file semantics.

chunks=None,
chunk_grid: bool = True,
use_dual: bool | None = False,
Expand All @@ -368,10 +368,12 @@ def open_dataset(
Strings and Path objects are interpreted as a path to a grid file. Xarray Datasets assume that
each member variable is in the UGRID conventions and will be used to create a ``ux.Grid``. Similarly, a dictionary
containing UGRID variables can be used to create a ``ux.Grid``
filename_or_obj : str | os.PathLike[Any]
filename_or_obj : str | os.PathLike[Any], optional
String or Path object as a path to a netCDF file or an OpenDAP URL that
stores the actual data set. It is the same ``filename_or_obj`` in
``xarray.open_dataset``.
``xarray.open_dataset``. If omitted, ``grid_filename_or_obj`` is also
used as the data source, allowing combined grid-and-data files to be
opened with a single argument.
chunks : int, dict, 'auto' or None, default: None
If provided, used to load the grid into dask arrays.

Expand Down Expand Up @@ -406,10 +408,22 @@ def open_dataset(

>>> import uxarray as ux
>>> ux_ds = ux.open_dataset("grid_file.nc", "data_file.nc")

Open a dataset stored in a single combined grid-and-data file

>>> ux_ds = ux.open_dataset("combined_file.nc")
"""
if grid_kwargs is None:
grid_kwargs = {}

if filename_or_obj is None:
if isinstance(grid_filename_or_obj, (str, os.PathLike)):
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.

Limiting what was originally possible (dict, xr.dataset) doesn't look like a great idea. I am not sure if we could support dict with this, but I am pretty sure we should for xr.dataset. I think the main check should be on whether a good Grid object could be opened out of the file object here or not.

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.

xr.Dataset should also work in the one-input case. I’ll update the implementation

filename_or_obj = grid_filename_or_obj
else:
raise ValueError(
"If filename_or_obj is omitted, grid_filename_or_obj must be a file path."
)

# Construct a Grid, validate parameters, and correct chunks
uxgrid, corrected_chunks = _get_grid(
grid_filename_or_obj, chunks, chunk_grid, use_dual, grid_kwargs, **kwargs
Expand Down
Loading