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
6 changes: 3 additions & 3 deletions dascore/core/coordmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -820,9 +820,8 @@ def equals(self, other) -> bool:
def shape(self):
"""Return the shape of the dimensions."""
out = tuple(len(self.coord_map[x]) for x in self.dims)
# empty arrays return (0,) as their shape, so we must do the same.
if not out:
return (0,)
return () if not self.dims else (0,)
return out

@property
Expand All @@ -837,7 +836,8 @@ def ndim(self):

def validate_data(self, data):
"""Ensure data conforms to coordinates."""
data = np.asarray([]) if data is None else data
if data is None:
data = np.empty(self.shape)
shape = tuple(data.shape)
if self.shape != shape:
msg = (
Expand Down
35 changes: 24 additions & 11 deletions dascore/io/dasdae/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def _save_patch(patch, wave_group, h5, name):
_save_attrs_and_dims(patch, patch_group)
_save_coords(patch, patch_group, h5)
# add data
if patch.data.shape:
if patch.data.shape or patch.data.ndim == 0:
_create_or_squash_array(h5, patch_group, "data", patch.data)


Expand Down Expand Up @@ -123,6 +123,14 @@ def _read_array(table_array):
return data


def _read_data_array(table_array):
"""Read patch data, including scalar arrays."""
try:
return table_array[:]
except IndexError:
return np.asarray(table_array[()])


def _get_coords(patch_group, dims, attrs2):
"""Get the coordinates from a patch group."""
coord_dict = {} # just store coordinates here
Expand Down Expand Up @@ -185,16 +193,21 @@ def _read_patch(patch_group, attrs=None, **kwargs):
# Note, previously this was wrapped with try, except (Index, KeyError)
# and the data = np.array(None) in except block. Not sure, why, removed
# try except.
if kwargs:
# We need to remove any coordinates from kwargs that are multi-dim
# coords.
cmap = coords.dim_map
sub_kwargs = {
i: v for i, v in kwargs.items() if (i not in cmap) or (len(cmap[i]) == 1)
}
coords, data = coords.select(array=patch_group["data"], **sub_kwargs)
else:
data = patch_group["data"][:]
try:
if kwargs:
# We need to remove any coordinates from kwargs that are multi-dim
# coords.
cmap = coords.dim_map
sub_kwargs = {
i: v
for i, v in kwargs.items()
if (i not in cmap) or (len(cmap[i]) == 1)
}
coords, data = coords.select(array=patch_group["data"], **sub_kwargs)
else:
data = _read_data_array(patch_group["data"])
except IndexError:
data = _read_data_array(patch_group["data"])
Comment on lines +196 to +210

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not swallow IndexError from coordinate selection.

This try block covers both coords.select(...) and HDF5 data loading. If selection raises IndexError, the handler returns the original coordinates with the complete, unfiltered data, silently ignoring the caller’s selector. Restrict the scalar-read retry to the data-read operation, or retry selection with materialized data and let a second selection error propagate.

Proposed fix
-    try:
-        if kwargs:
+    if kwargs:
             cmap = coords.dim_map
             sub_kwargs = {
                 i: v
                 for i, v in kwargs.items()
                 if (i not in cmap) or (len(cmap[i]) == 1)
             }
-            coords, data = coords.select(array=patch_group["data"], **sub_kwargs)
-        else:
-            data = _read_data_array(patch_group["data"])
-    except IndexError:
-        data = _read_data_array(patch_group["data"])
+            try:
+                coords, data = coords.select(
+                    array=patch_group["data"], **sub_kwargs
+                )
+            except IndexError:
+                data = _read_data_array(patch_group["data"])
+                coords, data = coords.select(array=data, **sub_kwargs)
+    else:
+        data = _read_data_array(patch_group["data"])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try:
if kwargs:
# We need to remove any coordinates from kwargs that are multi-dim
# coords.
cmap = coords.dim_map
sub_kwargs = {
i: v
for i, v in kwargs.items()
if (i not in cmap) or (len(cmap[i]) == 1)
}
coords, data = coords.select(array=patch_group["data"], **sub_kwargs)
else:
data = _read_data_array(patch_group["data"])
except IndexError:
data = _read_data_array(patch_group["data"])
if kwargs:
# We need to remove any coordinates from kwargs that are multi-dim
# coords.
cmap = coords.dim_map
sub_kwargs = {
i: v
for i, v in kwargs.items()
if (i not in cmap) or (len(cmap[i]) == 1)
}
try:
coords, data = coords.select(
array=patch_group["data"], **sub_kwargs
)
except IndexError:
data = _read_data_array(patch_group["data"])
coords, data = coords.select(array=data, **sub_kwargs)
else:
data = _read_data_array(patch_group["data"])
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/io/dasdae/utils.py` around lines 196 - 210, Update the data-loading
flow around coords.select and _read_data_array so IndexError from coordinate
selection is never swallowed. Limit the scalar-read fallback to errors from the
initial data read, or materialize the data and retry coords.select while
allowing any second selection error to propagate; preserve the caller’s
filtering arguments and avoid returning unfiltered data after selection failure.

return dc.Patch(data=data, coords=coords, dims=dims, attrs=attrs)
Comment thread
coderabbitai[bot] marked this conversation as resolved.


Expand Down
4 changes: 3 additions & 1 deletion dascore/proc/coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,9 @@ def squeeze(self: PatchType, dim=None) -> PatchType:
else:
axes = tuple(self.get_axis(x) for x in iterate(dim))
data = np.squeeze(self.data, axis=axes)
return self.new(data=data, coords=coords)
dims = coords.dims
attrs = self.attrs.new(dims=",".join(dims))
return self.new(data=data, coords=coords, dims=dims, attrs=attrs)


@patch_function()
Expand Down
12 changes: 8 additions & 4 deletions dascore/utils/attrs.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,19 @@ def _get_dims(obj):
return obj["coords"].dims

# This object already has dims, just honor it.
if dims := obj.get("dims", None):
return tuple(dims.split(",")) if isinstance(dims, str) else dims
if "dims" in obj:
dims_val = obj["dims"]
if isinstance(dims_val, str):
return tuple(dims_val.split(",")) if dims_val else ()
return tuple(dims_val)

potential_keys = defaultdict(set)
for key in obj:
if not is_valid_coord_str(key):
continue
potential_keys[key.split("_")[0]].add(key.split("_")[1])
return tuple(i for i, v in potential_keys.items() if _meets_required(v))
found = tuple(i for i, v in potential_keys.items() if _meets_required(v))
return found if found else None

def _get_coords_from_top_level(obj, out, dims):
"""First get coord info from top level."""
Expand Down Expand Up @@ -289,7 +293,7 @@ def _pop_keys(obj, out):
obj = dict(obj)
# Check if dims need to be updated.
new_dims = _get_dims(obj)
if new_dims and new_dims != dims:
if new_dims is not None and new_dims != dims:
obj["dims"] = new_dims
dims = new_dims
# this is already a dict of coord info.
Expand Down
4 changes: 2 additions & 2 deletions tests/test_core/test_coordmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ def test_empty(self):
coord = get_coord_manager()
assert isinstance(coord, CoordManager)
assert dict(coord) == {}
# shape should be the same as an empty array.
assert coord.shape == np.array([]).shape
# an empty coord manager has no dims, so shape is () like a 0-D array.
assert coord.shape == ()

def test_str(self, coord_manager):
"""Tests the str output for coord manager."""
Expand Down
20 changes: 20 additions & 0 deletions tests/test_proc/test_proc_coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,26 @@ def test_noop_coord_squeeze_returns_self(self, random_patch):
"""CoordManager squeeze with no length-1 dims returns self."""
coords = random_patch.coords
assert coords.squeeze() is coords
def test_squeeze_single_dim_on_1x1_patch(self):
"""Squeeze one dim of a (1,1) patch should work (#623)."""
data = np.arange(1, dtype=np.float64).reshape(1, 1)
patch = dc.Patch(
data=data,
coords={
"distance": (("distance",), np.array([0.0])),
"time": (("time",), np.array([0.0])),
},
dims=["distance", "time"],
)
out = patch.squeeze("distance")
assert out.dims == ("time",)
assert out.attrs.dim_tuple == ("time",)
assert out.shape == (1,)

out_all = patch.squeeze()
assert out_all.dims == ()
assert out_all.attrs.dim_tuple == ()
assert out_all.shape == ()


class TestGetCoord:
Expand Down