Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
26 changes: 26 additions & 0 deletions test/core/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,32 @@ 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."""

# Use a known combined grid-and-data MPAS file.
file_path = gridpath("mpas", "QU", "oQU480.231010.nc")

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

# Ensure that the single-argument path actually loads data variables
assert len(uxds_single.data_vars) > 0
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))
assert "ssh" in uxds_single.data_vars


def test_open_dataset_single_argument_rejects_directory_grid(tmp_path):
"""Requires a separate data file for directory-based grids."""

with pytest.raises(ValueError, match="Directory-based grids require a separate data file"):
ux.open_dataset(tmp_path)


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
53 changes: 44 additions & 9 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.

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,17 +408,50 @@ 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 = {}

# 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
)
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.

if os.path.isdir(grid_filename_or_obj):
raise ValueError(
"Directory-based grids require a separate data file when calling ux.open_dataset()."
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 to cover the multiple grid files case?

)

# Load the data as a Xarray Dataset
ds = _open_dataset_with_fallback(filename_or_obj, chunks=corrected_chunks, **kwargs)
filename_or_obj = grid_filename_or_obj

if "latlon" in kwargs:
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.

Why this check?

warn(
"'latlon is no longer a supported parameter",
DeprecationWarning,
stacklevel=2,
)
grid_kwargs["latlon"] = kwargs["latlon"]

corrected_chunks = match_chunks_to_ugrid(grid_filename_or_obj, chunks)
ds = _open_dataset_with_fallback(
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 am a bit confused about _get_grid vs. match_chunks_to_ugrid plus _open_dataset_with_fallback. Please clarify why this switch.

filename_or_obj, chunks=corrected_chunks, **kwargs
)
uxgrid = open_grid(ds, use_dual=use_dual, **grid_kwargs)
else:
raise ValueError(
"If filename_or_obj is omitted, grid_filename_or_obj must be a file path."
)
else:
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.

Maybe rather than splitting the whole logic into two parts as it is in this if-else block now, we'd only need to split the grid opening task as follows:

  1. Check if I have only one file path object given
    • If so, try & catch opening a grid object from that one
    • Else, try opening the grid from the separate grid file
  2. In both cases, open dataset from the same file

# 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
)

# Load the data as a Xarray Dataset
ds = _open_dataset_with_fallback(
filename_or_obj, chunks=corrected_chunks, **kwargs
)

# Map original dimensions to the UGRID conventions
ds = _map_dims_to_ugrid(ds, uxgrid._source_dims_dict, uxgrid)
Expand Down
Loading