While addressing #1556 I noticed a variety of changes I would recommend making to error types being used, to make them correspond more closely to the actual error being reported. For example, when an error is raised because an object was expected to be an xr.DataArray or a dask.Array, but it is not either of those types, the error should be a TypeError, not a ValueError.
Here is an exhaustive list of all such cases, split into two sections. First, recommended changes which would not break backwards compatibility; the recommended type is a subclass of the currently-used type. Second, recommended changes where the recommended type is not a subclass of the currently-used type.
EDIT: updated the second section to include changes originally recommended in #1621 but which did not end up getting included in that PR.
EDIT: also note: work should only begin on this issue after at least one other uxarray developer confirms these changes sound reasonable. It will also be necessary to decide if these changes need to be incorporated with some sort of deprecation cycle. I personally would lean towards not having a deprecation cycle, because: (A) these are already error messages anyway, (B) I would be surprised if many people are running code that relies directly on the type of errors being raised by uxarray, and (C) getting a deprecation warning during a crash sounds quite confusing, and might make it harder to quickly understand the actual cause of the crash.
Proposed changes which do not break backwards compatibility:
- Exception → ValueError in grid.bounds.insert_pt_in_latlonbox: "Longitude point out of range"
- Exception → AssertionError or ValueError(?) in grid.bounds.insert_pt_in_latlonbox: "Logic error in longitude box width calculation"
- Exception → AttributeError in property Grid.inverse_indices: "Grid is not a subset, therefore no inverse face indices exist"
Proposed changes which do break backwards compatibility:
- ValueError → TypeError in core.aggregation._node_to_face_aggregation and core.aggregation._node_to_edge_aggregation: (error currently has no message, but is raised to indicate: not isinstance(uxda.data, (np.ndarray, da.Array)).)
- ValueError → TypeError in UxDataArray.zonal_anomaly: "Invalid value for 'lat'. Must be a tuple (start, end, step) or array-like band edges."
- ValueError → TypeError in Grid.from_face_vertices: "Input must be either a list, tuple, or np.ndarray"
- ValueError → NotImplementedError in Grid.boundary_node_indices: (error currently has no message, but seems like it is supposed to indicate that construction of boundary_node_indices is not yet implemented.)
- TypeError → ValueError in grid.neighbors.KDTree._build_from_nodes: f"Unknown coordinate_system, {self.coordinate_system}, use either 'cartesian' or 'spherical'".
- TypeError → ValueError in grid.neighbors.KDTree._current_tree and grid.neighbors.BallTree._current_tree: f"Unknown coordinates location, {self._coordinates}, use either 'nodes', 'face centers', or 'edge centers'"
- ValueError → NotImplementedError in grid.slice._slice_node_indices, grid.slice._slice_node_indices, and grid.slice._slice_face_indices: "Exclusive slicing is not yet supported."
- ValueError → TypeError in grid.utils.make_setter: f"{key} must be an xr.DataArray" (raised if not isinstance(value, xr.DataArray)).
- ValueError → NotImplementedError in io._esmf._read_esmf: "Reading in ESMF grids with Cartesian coordinates not yet supported"
- ValueError → NotImplementedError in io._icon._read_icon: "Conversion of the ICON Dual mesh is not yet supported."
- Exception → NotImplementedError in io._scrip._to_ugrid: "Structured scrip files are not yet supported"
- ValueError → AttributeError in remap.yac._get_lon_lat: f"Grid does not provide {lon_attr}/{lat_attr} required for YAC remapping."
And, adding the changes originally recommended in #1621:
- RuntimeError → GridInvalidError, in UxDataArray.get_dual, UxDataset.get_dual, and Grid.get_dual: "Duplicate nodes found, cannot construct dual"
- RuntimeError → DimensionError, in Grid.from_face_vertices: f"Invalid Input Dimension: {face_vertices.ndim}. Expected dimension should be 3: [n_face, n_node, two/three] or 2 when only one face is passed in."
- RuntimeError → GridInvalidError, in Grid.validate: "Mesh validation failed."
- AssertionError → DimensionError, in grid.neighbors._prepare_xy_for_query: "The dimension of each coordinate pair must be two (lon, lat). Did you attempt to query using Cartesian (x, y, z) coordinates?", and "The dimension of each coordinate pair must be two (lon, lat).)"
- AssertionError → DimensionError, in grid.neighbors._prepare_xyz_for_query: (similar messages as above)
- RuntimeError → GridInvalidError, in io.utils._parse_grid_type: "Failed to parse uxgrid information from xarray.Dataset."
While addressing #1556 I noticed a variety of changes I would recommend making to error types being used, to make them correspond more closely to the actual error being reported. For example, when an error is raised because an object was expected to be an xr.DataArray or a dask.Array, but it is not either of those types, the error should be a TypeError, not a ValueError.
Here is an exhaustive list of all such cases, split into two sections. First, recommended changes which would not break backwards compatibility; the recommended type is a subclass of the currently-used type. Second, recommended changes where the recommended type is not a subclass of the currently-used type.
EDIT: updated the second section to include changes originally recommended in #1621 but which did not end up getting included in that PR.
EDIT: also note: work should only begin on this issue after at least one other uxarray developer confirms these changes sound reasonable. It will also be necessary to decide if these changes need to be incorporated with some sort of deprecation cycle. I personally would lean towards not having a deprecation cycle, because: (A) these are already error messages anyway, (B) I would be surprised if many people are running code that relies directly on the type of errors being raised by uxarray, and (C) getting a deprecation warning during a crash sounds quite confusing, and might make it harder to quickly understand the actual cause of the crash.
Proposed changes which do not break backwards compatibility:
Proposed changes which do break backwards compatibility:
And, adding the changes originally recommended in #1621: