Repository: tomvothecoder/xcdat

Issue:
Separate getting and adding bounds to avoid implicit side effects
### Is your feature request related to a problem? If yes, Please describe
<!-- Concise description of the problem -->
After reviewing the below lines, I realized that it is not best practice to perform implicit state change of objects, while simultaneously returning information about the object. This can lead to confusing behavior with functions/methods.
The `allow_generating` boolean flag performs the implicit state change.

https://github.com/tomvothecoder/xcdat/blob/62e15fb2b5ea387359a678df0f5259dd1f58ddfb/xcdat/bounds.py#L59-L111


### Describe your proposed solution
<!-- Concise description of your proposed solution. -->
Separate `get_bounds()` to `get_bounds()` and `guess_bounds()`.

- `get_bounds()` retrieves information about the Dataset (the bounds)
- `guess_bounds()` changes the state of the Dataset by adding bounds

### Describe alternatives solutions you've considered
<!-- Concise description of any alternative solutions or features you've considered. -->
N/A

### Additional context
<!-- Other context, examples, or screenshots about the feature request here. -->

Acceptance Criteria

- [x]  `get_bounds()` returns bounds for a coordinate (use cf_xarray)
   - If they don't exist, raise KeyError with message to generate bounds
   - Refer to https://github.com/xarray-contrib/cf-xarray/blob/main/cf_xarray/accessor.py#L1877
- [x] `add_bounds()` adds bounds for a coordinate 
   - If they exist, user must first drop existing bounds 
   - If they don't exist, run `_guess_bounds()`
- [x] Add property `bounds`, which is a dict that maps coordinate keys to bounds DataArrays

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo) Report
> Merging [#94](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo) (abf723e) into [main](https://codecov.io/gh/tomvothecoder/xcdat/commit/62e15fb2b5ea387359a678df0f5259dd1f58ddfb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo) (62e15fb) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/tomvothecoder/xcdat/pull/94/graphs/tree.svg?width=650&height=150&src=pr&token=T8W41LN26O&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo)](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo)

```diff
@@            Coverage Diff            @@
##              main       #94   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          165       180   +15     
=========================================
+ Hits           165       180   +15     
```


| [Impacted Files](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo) | Coverage Δ | |
|---|---|---|
| [xcdat/bounds.py](https://codecov.io/gh/tomvothecoder/xcdat/pull/94/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo#diff-eGNkYXQvYm91bmRzLnB5) | `100.00% <100.00%> (ø)` | |
| [xcdat/dataset.py](https://codecov.io/gh/tomvothecoder/xcdat/pull/94/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo#diff-eGNkYXQvZGF0YXNldC5weQ==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo). Last update [62e15fb...abf723e](https://codecov.io/gh/tomvothecoder/xcdat/pull/94?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tom+Vo).


Patch:
diff --git a/xcdat/bounds.py b/xcdat/bounds.py
index 197c74f..1cc030e 100644
--- a/xcdat/bounds.py
+++ b/xcdat/bounds.py
@@ -1,4 +1,5 @@
 """Bounds module for functions related to coordinate bounds."""
+import collections
 from typing import Dict, Hashable, List, Optional, Tuple, get_args
 
 import cf_xarray as cfxr  # noqa: F401
@@ -26,25 +27,58 @@ class DatasetBoundsAccessor:
 
     >>> from xcdat import bounds
 
-    Get coordinate bounds if they exist, otherwise return generated bounds:
+    Return dictionary of coordinate keys mapped to bounds DataArrays:
 
     >>> ds = xr.open_dataset("file_path")
+    >>> bounds = ds.bounds.bounds
+
+    Fill missing coordinate bounds in the Dataset:
+
+    >>> ds = xr.open_dataset("file_path")
+    >>> ds = ds.bounds.fill_missing()
+
+    Get coordinate bounds if they exist:
+
+    >>> ds = xr.open_dataset("file_path")
+    >>>
+    >>> # Throws error if bounds don't exist
     >>> lat_bounds = ds.bounds.get_bounds("lat") # or pass "latitude"
     >>> lon_bounds = ds.bounds.get_bounds("lon") # or pass "longitude"
     >>> time_bounds = ds.bounds.get_bounds("time")
 
-    Get coordinate bounds and don't generate if they don't exist:
+    Add coordinates bounds if they don't exist:
 
     >>> ds = xr.open_dataset("file_path")
-    >>> # Throws error if no bounds exist
-    >>> lat_bounds = ds.bounds.get_bounds("lat", allow_generating=False)
+    >>>
+    >>> # Throws error if bounds exist
+    >>> ds = ds.bounds.add_bounds("lat")
     """
 
     def __init__(self, dataset: xr.Dataset):
         self._dataset: xr.Dataset = dataset
 
-    def get_bounds_for_all_coords(self, allow_generating=True) -> xr.Dataset:
-        """Gets existing bounds or generates new ones for supported coordinates.
+    @property
+    def bounds(self) -> Dict[str, Optional[xr.DataArray]]:
+        """Returns a mapping of coordinate and axis keys to their bounds.
+
+        The dictionary provides all valid CF compliant keys for a coordinate.
+        For example, latitude will includes keys for "lat", "latitude", and "Y".
+
+        Returns
+        -------
+        Dict[str, Optional[xr.DataArray]]
+            Dictionary mapping coordinate keys to their bounds.
+        """
+        ds = self._dataset
+
+        bounds: Dict[str, Optional[xr.DataArray]] = {}
+        for coord, bounds_name in ds.cf.bounds.items():
+            bounds[coord] = ds.get(bounds_name, None)
+
+        return collections.OrderedDict(sorted(bounds.items()))
+
+    def fill_missing(self) -> xr.Dataset:
+        """Fills any missing bounds for supported coordinates in the Dataset.
 
         Returns
         -------
@@ -52,29 +86,20 @@ class DatasetBoundsAccessor:
         """
         for coord in [*self._dataset.coords]:
             if coord in SUPPORTED_COORDS:
-                self.get_bounds(coord, allow_generating)
+                try:
+                    self._dataset.cf.get_bounds(coord)
+                except KeyError:
+                    self._dataset = self.add_bounds(coord)
 
         return self._dataset
 
-    def get_bounds(
-        self,
-        coord: Coord,
-        allow_generating: bool = True,
-        width: float = 0.5,
-    ) -> Optional[xr.DataArray]:
-        """Get bounds for a coordinate, or generate bounds if it doesn't exist.
+    def get_bounds(self, coord: Coord) -> xr.DataArray:
+        """Get bounds for a coordinate.
 
         Parameters
         ----------
         coord : Coord
-            The "lat" or "lon" coordinate.
-        allow_generating : bool, optional
-            If True, generate bounds if they don't exist. If False, return
-            only existing bounds or throw error if they don't exist, by default
-            True.
-        width : float, optional
-            Represents the width of the bounds relative to the position of the
-            points if generating bounds, by default 0.5.
+            The coordinate key.
 
         Returns
         -------
@@ -85,8 +110,9 @@ class DatasetBoundsAccessor:
         ------
         ValueError
             If an incorrect ``coord`` argument is passed.
+
         ValueError
-            If ``allow_generating=False`` and no bounds were found in the dataset.
+            If bounds were not found in the dataset. They must be added.
         """
         if coord not in SUPPORTED_COORDS:
             raise ValueError(
@@ -97,38 +123,60 @@ class DatasetBoundsAccessor:
         try:
             bounds = self._dataset.cf.get_bounds(coord)
         except KeyError:
-            bounds = None
-            if bounds is None and not allow_generating:
-                raise ValueError(
-                    f"{coord} bounds were not found in the dataset, bounds must be generated"
-                )
-
-            logger.info(
-                f"{coord} bounds were not found in the dataset, generating bounds."
+            raise KeyError(
+                f"{coord} bounds were not found in the dataset, they must be added."
             )
-            bounds = self._generate_bounds(coord, width)
 
         return bounds
 
-    def _generate_bounds(
-        self,
-        coord: Coord,
-        width: float = 0.5,
-    ) -> xr.DataArray:
-        """Generates bounds for a coordinate using its data points.
+    def add_bounds(self, coord: Coord, width: float = 0.5) -> xr.Dataset:
+        """Add bounds for a coordinate using its data points.
+
+        If bounds already exist, they must be dropped first.
 
         Parameters
         ----------
         coord : Coord
-            The coordinate.
+            The coordinate key.
         width : float, optional
-            Width of the bounds relative to the position of the nearest
-            points, by default 0.5.
+            Width of the bounds relative to the position of the nearest points,
+            by default 0.5.
 
         Returns
         -------
-        xr.DataArray
-            The coordinate bounds.
+        xr.Dataset
+            The dataset with bounds added.
+
+        Raises
+        ------
+        ValueError
+            If bounds already exist. They must be dropped first.
+        """
+        try:
+            self._dataset.cf.get_bounds(coord)
+            raise ValueError(
+                f"{coord} bounds already exist. Drop them first to add new bounds."
+            )
+        except KeyError:
+            dataset = self._add_bounds(coord, width)
+
+        return dataset
+
+    def _add_bounds(self, coord: Coord, width: float = 0.5) -> xr.Dataset:
+        """Adds bounds for a coordinate using its data points.
+
+        Parameters
+        ----------
+        coord : Coord
+            The coordinate key.
+        width : float, optional
+            Width of the bounds relative to the position of the nearest points,
+            by default 0.5.
+
+        Returns
+        -------
+        xr.Dataset
+            The dataset with bounds added.
 
         Raises
         ------
@@ -179,17 +227,18 @@ class DatasetBoundsAccessor:
                 np.clip(bounds, -90, 90, out=bounds)
 
         # Add coordinate bounds to the dataset
+        dataset = self._dataset.copy()
         var_name = f"{coord}_bnds"
-        self._dataset[var_name] = xr.DataArray(
+        dataset[var_name] = xr.DataArray(
             name=var_name,
             data=bounds,
             coords={coord: da_coord},
             dims=[coord, "bnds"],
             attrs={"is_generated": "True"},
         )
-        self._dataset[da_coord.name].attrs["bounds"] = var_name
+        dataset[da_coord.name].attrs["bounds"] = var_name
 
-        return self._dataset[var_name]
+        return dataset
 
     def _get_coord(self, coord: Coord) -> xr.DataArray:
         """Get the matching coordinate in the dataset.
@@ -197,7 +246,7 @@ class DatasetBoundsAccessor:
         Parameters
         ----------
         coord : Coord
-            The coordinate.
+            The coordinate key.
 
         Returns
         -------
@@ -228,15 +277,28 @@ class DataArrayBoundsAccessor:
     >>> from xcdat import bounds
     >>> from xcdat.dataset import open_dataset
 
-    Return attribute (refer to ``tas.bounds__dict__`` for attributes):
-
-    >>> tas.bounds.<attribute>
-
-    Copy axis bounds from parent Dataset to data variable:
+    Copy coordinate bounds from parent Dataset to data variable:
 
     >>> ds = open_dataset("file_path") # Auto-generates bounds if missing
     >>> tas = ds["tas"]
     >>> tas.bounds._copy_from_dataset(ds)
+
+    Return dictionary of coordinate keys mapped to bounds DataArrays:
+
+    >>> tas.bounds.bounds
+
+    Return dictionary of coordinate keys mapped to bounds names:
+
+    >>> tas.bounds.bounds_names
+
+    Get bounds for a coordinate key:
+
+    >>> tas.bounds.get_bounds("lat")
+
+    Get name of bounds dimension:
+
+    >>> tas.bounds.get_bounds_dim_name("lat")
+
     """
 
     def __init__(self, dataarray: xr.DataArray):
@@ -398,7 +460,7 @@ class DataArrayBoundsAccessor:
         Parameters
         ----------
         coord : Coord
-            Name of the coordinate whose bounds are desired.
+            The coordinate key whose bounds are desired.
 
         Returns
         -------
@@ -418,7 +480,7 @@ class DataArrayBoundsAccessor:
         Parameters
         ----------
         coord : Coord
-            Name of the coordinate whose bounds dimension is desired.
+            The coordinate key whose bounds dimension is desired.
 
         Returns
         -------
diff --git a/xcdat/dataset.py b/xcdat/dataset.py
index 9391ada..7970666 100644
--- a/xcdat/dataset.py
+++ b/xcdat/dataset.py
@@ -55,7 +55,7 @@ def open_dataset(path: str, **kwargs: Dict[str, Any]) -> xr.Dataset:
     """
     ds = xr.open_dataset(path, decode_times=False, **kwargs)
     ds = decode_time_units(ds)
-    ds = ds.bounds.get_bounds_for_all_coords()
+    ds = ds.bounds.fill_missing()
 
     return ds
 
@@ -115,7 +115,7 @@ def open_mfdataset(paths: Union[str, List[str]], **kwargs) -> xr.Dataset:
     """
     ds = xr.open_mfdataset(paths, decode_times=False, **kwargs)
     ds = decode_time_units(ds)
-    ds = ds.bounds.get_bounds_for_all_coords()
+    ds = ds.bounds.fill_missing()
 
     return ds
 


Test patch:
diff --git a/tests/test_bounds.py b/tests/test_bounds.py
index 238f76b..702b0f8 100644
--- a/tests/test_bounds.py
+++ b/tests/test_bounds.py
@@ -2,7 +2,14 @@ import numpy as np
 import pytest
 import xarray as xr
 
-from tests.fixtures import generate_dataset, ts_cf, ts_with_bnds_from_parent_cf
+from tests.fixtures import (
+    generate_dataset,
+    lat_bnds,
+    lon_bnds,
+    time_bnds,
+    ts_cf,
+    ts_with_bnds_from_parent_cf,
+)
 from xcdat.bounds import DataArrayBoundsAccessor, DatasetBoundsAccessor
 
 
@@ -10,6 +17,7 @@ class TestDatasetBoundsAccessor:
     @pytest.fixture(autouse=True)
     def setup(self):
         self.ds = generate_dataset(cf_compliant=True, has_bounds=False)
+        self.ds_with_bnds = generate_dataset(cf_compliant=True, has_bounds=True)
 
     def test__init__(self):
         obj = DatasetBoundsAccessor(self.ds)
@@ -18,51 +26,62 @@ class TestDatasetBoundsAccessor:
     def test_decorator_call(self):
         assert self.ds.bounds._dataset.identical(self.ds)
 
-    def test_get_bounds_when_bounds_exist_in_dataset(self):
-        ds = generate_dataset(cf_compliant=True, has_bounds=True)
-        lat_bnds = ds.bounds.get_bounds("lat")
-        assert lat_bnds.identical(ds.lat_bnds)
+    def test_bounds_property_returns_map_of_coordinate_key_to_bounds_dataarray(self):
+        ds = self.ds_with_bnds.copy()
+        expected = {
+            "T": ds.time,
+            "X": ds.lon,
+            "Y": ds.lat,
+            "lat": ds.lat,
+            "latitude": ds.lat,
+            "lon": ds.lon,
+            "longitude": ds.lon,
+            "time": ds.time,
+        }
+
+        result = ds.bounds.bounds
 
-        lon_bnds = ds.bounds.get_bounds("lon")
-        assert lon_bnds.identical(ds.lon_bnds)
-        assert lon_bnds.is_generated
+        assert result == expected
 
-    def test_get_bounds_when_bounds_do_not_exist_in_dataset(self):
-        ds = self.ds.copy()
+    def test_fill_missing_returns_dataset_with_filled_bounds(self):
+        ds = self.ds_with_bnds.copy()
 
+        ds = ds.drop_vars(["lat_bnds", "lon_bnds"])
+
+        result = ds.bounds.fill_missing()
+        assert result.identical(self.ds_with_bnds)
+
+    def test_get_bounds_returns_error_when_bounds_dont_exist(self):
+        with pytest.raises(KeyError):
+            self.ds.bounds.get_bounds("lat")
+
+    def test_get_bounds_when_bounds_exist_in_dataset(self):
+        ds = self.ds_with_bnds.copy()
         lat_bnds = ds.bounds.get_bounds("lat")
-        assert lat_bnds is not None
         assert lat_bnds.identical(ds.lat_bnds)
-        assert lat_bnds.is_generated
 
         lon_bnds = ds.bounds.get_bounds("lon")
-        assert lon_bnds is not None
         assert lon_bnds.identical(ds.lon_bnds)
         assert lon_bnds.is_generated
 
-        # Check raises error when bounds do not exist and not allowing generated bounds.
-        with pytest.raises(ValueError):
+    def test_get_bounds_when_bounds_do_not_exist_in_dataset(self):
+        ds = self.ds_with_bnds.copy()
+
+        with pytest.raises(KeyError):
             ds = ds.drop_vars(["lat_bnds"])
-            ds.bounds.get_bounds("lat", allow_generating=False)
+            ds.bounds.get_bounds("lat")
 
-    def test_get_bounds_raises_error_with_incorrect_axis_argument(self):
+    def test_get_bounds_raises_error_with_incorrect_coord_argument(self):
         with pytest.raises(ValueError):
-            self.ds.bounds.get_bounds("incorrect_axis_argument")
+            self.ds.bounds.get_bounds("incorrect_coord_argument")
 
-    def test__get_bounds_does_not_drop_attrs_of_existing_coords_when_generating_bounds(
-        self,
-    ):
-        ds_expected = self.ds.copy()
-        ds_expected["lat"].attrs["bounds"] = "lat_bnds"
-
-        ds_result = self.ds.copy()
-        lat_bnds = ds_result.bounds.get_bounds("lat", allow_generating=True)
-        assert lat_bnds.identical(ds_result.lat_bnds)
+    def test_add_bounds_raises_error_if_bounds_exist(self):
+        ds = self.ds_with_bnds.copy()
 
-        ds_result = ds_result.drop_vars("lat_bnds")
-        assert ds_result.identical(ds_expected)
+        with pytest.raises(ValueError):
+            ds.bounds.add_bounds("lat")
 
-    def test__generate_bounds_raises_errors_for_data_dim_and_length(self):
+    def test__add_bounds_raises_errors_for_data_dim_and_length(self):
         # Multidimensional
         lat = xr.DataArray(
             data=np.array([[0, 1, 2], [3, 4, 5]]),
@@ -79,22 +98,26 @@ class TestDatasetBoundsAccessor:
 
         # If coords dimensions does not equal 1.
         with pytest.raises(ValueError):
-            ds.bounds._generate_bounds("lat")
+            ds.bounds._add_bounds("lat")
         # If coords are length of <=1.
         with pytest.raises(ValueError):
-            ds.bounds._generate_bounds("lon")
+            ds.bounds._add_bounds("lon")
 
-    def test__generate_bounds_returns_bounds(self):
+    def test__add_bounds_returns_dataset_with_bounds_added(self):
         ds = self.ds.copy()
 
-        lat_bnds = ds.bounds._generate_bounds("lat")
-        assert lat_bnds.equals(ds.lat_bnds)
+        ds = ds.bounds._add_bounds("lat")
+        assert ds.lat_bnds.equals(lat_bnds)
         assert ds.lat_bnds.is_generated
 
-        lon_bnds = ds.bounds._generate_bounds("lon")
-        assert lon_bnds.equals(ds.lon_bnds)
+        ds = ds.bounds._add_bounds("lon")
+        assert ds.lon_bnds.equals(lon_bnds)
         assert ds.lon_bnds.is_generated
 
+        ds = ds.bounds._add_bounds("time")
+        assert ds.time_bnds.equals(time_bnds)
+        assert ds.time_bnds.is_generated
+
     def test__get_coord(self):
         ds = self.ds.copy()
 
diff --git a/tests/test_dataset.py b/tests/test_dataset.py
index 5895bc5..7f8f228 100644
--- a/tests/test_dataset.py
+++ b/tests/test_dataset.py
@@ -24,7 +24,7 @@ class TestOpenDataset:
 
         result_ds = open_dataset(self.file_path)
         # Replicates decode_times=False, which adds units to "time" coordinate.
-        # Refer to xcdat.bounds.DatasetBoundsAccessor._generate_bounds() for
+        # Refer to xcdat.bounds.DatasetBoundsAccessor._add_bounds() for
         # how attributes propagate from coord to coord bounds.
         result_ds["time_bnds"].attrs["units"] = "months since 2000-01-01"
 
@@ -91,7 +91,7 @@ class TestOpenMfDataset:
 
         result_ds = open_mfdataset([self.file_path1, self.file_path2])
         # Replicates decode_times=False, which adds units to "time" coordinate.
-        # Refer to xcdat.bounds.DatasetBoundsAccessor._generate_bounds() for
+        # Refer to xcdat.bounds.DatasetBoundsAccessor._add_bounds() for
         # how attributes propagate from coord to coord bounds.
         result_ds.time_bnds.attrs["units"] = "months since 2000-01-01"