Skip to content
Merged
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
9 changes: 7 additions & 2 deletions dascore/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,13 @@ def map(self, func, iterables, **kwargs):
# Options for handling specific warnings
WARN_LEVELS = Literal["warn", "raise", None]

# A map from the unit name to the code used in numpy.timedelta64
NUMPY_TIME_UNIT_MAPPING = {
# A map from the unit name to the code used in numpy.timedelta64. The codes
# are spelled out in the annotation because numpy's unit parameter accepts
# only those literals, not str.
NUMPY_TIME_UNIT_MAPPING: Mapping[
str,
Literal["h", "m", "s", "ms", "us", "ns", "ps", "fs", "as", "Y", "M", "W", "D"],
] = {
"hour": "h",
"minute": "m",
"second": "s",
Expand Down
11 changes: 10 additions & 1 deletion dascore/core/coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ def to_coord(self) -> CoordRange:
msg = "Cannot convert summary which is not evenly sampled to coord."
raise CoordError(msg)
step = self.step
assert step is not None # is_range_like above rules out a null step
# this is a reverse coord
if np.sign(step) == -1:
start, stop = self.max, self.min + step
Expand Down Expand Up @@ -288,7 +289,11 @@ class BaseCoord(DascoreBaseModel, abc.ABC):

units: UnitQuantity = None
step: Any = None
shape: tuple[int, ...] | None = None
# Every coord has a shape; each subclass derives it in a before-validator
# from the values or range it was built with. The default exists only
# because those validators are invisible to type checkers, which would
# otherwise want shape passed at every construction site.
shape: tuple[int, ...] = ()
dtype: Any = None

if TYPE_CHECKING:
Expand Down Expand Up @@ -1186,6 +1191,10 @@ class CoordPartial(BaseCoord):
A coordinate which only contains partial information.
"""

# Redeclared without a default: a partial coord is nothing but its
# shape, and it is the one coord which cannot re-derive it on the way
# back from a model_dump(exclude_defaults=True).
shape: tuple[int, ...]
start: Any = np.nan
stop: Any = np.nan
step: Any = np.nan
Expand Down
6 changes: 5 additions & 1 deletion dascore/core/spool.py
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,11 @@ def split(
msg = "Spool.split requires either spool_count or spool_size."
raise ParameterError(msg)
start = 0
step = int(np.ceil(len(self) / count if count else size))
if count is not None:
step = int(np.ceil(len(self) / count))
else:
assert size is not None # the check above sets exactly one of them
step = int(np.ceil(size)) # tolerate a non-integral size
Comment on lines +587 to +591

@coderabbitai coderabbitai Bot Aug 4, 2026

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.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Reject non-positive split parameters before computing step.

Line 588 divides by count, so count=0 raises ZeroDivisionError. Line 591 can produce step=0 for size=0. Lines 592-594 then never advance. Negative values can also make the generator run forever. Validate that both parameters are greater than zero and raise ParameterError.

Proposed fix
         if not ((count is not None) ^ (size is not None)):
             msg = "Spool.split requires either spool_count or spool_size."
             raise ParameterError(msg)
+        if count is not None and count <= 0:
+            raise ParameterError("count must be greater than zero.")
+        if size is not None and size <= 0:
+            raise ParameterError("size must be greater than zero.")
         start = 0
🤖 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/core/spool.py` around lines 587 - 591, Validate the split parameters
before the step calculation in the affected spool splitting method: reject any
provided count or size that is less than or equal to zero by raising
ParameterError. Ensure this validation occurs before computing step so count=0,
size=0, and negative values cannot produce invalid or non-advancing iteration.

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.

Real, but out of scope here and pre-existing: size=0 and negative sizes have produced a non-terminating generator since the method was written, and this PR did not touch that path. The only thing that changed is count=0, which now raises ZeroDivisionError where it previously raised TypeError — both unhelpful. Guarding the parameters means adding new ParameterError paths to a public API, which wants its own PR and tests rather than riding along in a type-checking change. Tracking it separately.

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.

@d-chambers, acknowledged. This PR should remain focused on the type-checking changes.

A follow-up change can define the public behavior for non-positive count and size, then add the required tests. I will leave this finding unresolved for the separately tracked work.

You are interacting with an AI system.

while start < len(self):
yield self[start : start + step]
start += step
Expand Down
12 changes: 9 additions & 3 deletions dascore/io/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -777,8 +777,14 @@ def __call__(self, *args, **kwargs): ...


def _required_resource_type(method) -> type | None:
"""Return the resource type a FiberIO method's caster coerces its input to."""
return cast(_TypeCasterMethod, method)._required_type
"""
Return the resource type a FiberIO method's caster coerces its input to.

None when the method's resource parameter carries no type hint, or
when the method was never wrapped at all (only the base FiberIO's
own methods, which __init_subclass__ does not visit).
"""
return getattr(method, "_required_type", None)


def _type_caster(func, sig, required_type, arg_name):
Expand Down Expand Up @@ -1218,7 +1224,7 @@ def _get_fiber_io_and_req_type(
fiber_io_hint = FiberIO.manager.get_fiberio(
format=file_format_, version=file_version_
)
req_type = getattr(fiber_io_hint.scan, "_required_type", None)
req_type = _required_resource_type(fiber_io_hint.scan)
resource = manager.get_resource(req_type)
# this will get the required resource type to pass to scan.
return fiber_io_hint, resource
Expand Down
2 changes: 1 addition & 1 deletion dascore/io/index/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def _coerce_scalar(value, target_kinds: set[str]):
raise InvalidSpoolQueryError(msg)
if typed.kind == "str" and "time" in target_kinds:
try:
retyped = typed_value(np.datetime64(pd.Timestamp(value), "ns"))
retyped = typed_value(pd.Timestamp(value).to_datetime64())
return retyped
except (ValueError, TypeError):
pass
Expand Down
10 changes: 5 additions & 5 deletions dascore/io/sintela/protobuf_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -580,12 +580,12 @@ def _base_attrs(
def _get_band_attr_data_type(band_def: tuple[tuple[Any, ...], ...]) -> tuple[str, str]:
"""Return patch-level BAND data type/units."""
mapped = [_BAND_DATA_TYPE_MAP.get(int(item[0])) for item in band_def]
if any(item is None for item in mapped):
return "frequency_band_energy", ""
# Only bands which all map to the same known data type carry its units;
# an unmapped band is None, which no mapped band compares equal to.
first = mapped[0]
if all(item == first for item in mapped):
return "frequency_band_energy", first[1]
return "frequency_band_energy", ""
if first is None or any(item != first for item in mapped):
return "frequency_band_energy", ""
return "frequency_band_energy", first[1]


def _assert_equal(name: str, values: list[Any]):
Expand Down
26 changes: 16 additions & 10 deletions dascore/utils/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import importlib
import inspect
import itertools
import math
import os
import re
import warnings
Expand Down Expand Up @@ -770,7 +771,8 @@ def _spool_map(spool, func, size=None, client=None, progress=True, **kwargs):
# Now things get interesting. We need to split the spool here
# so that patches don't get serialized.
if size is None:
size = len(spool) / (os.cpu_count() or 1)
# split takes a patch count, so round up rather than hand it a float.
size = math.ceil(len(spool) / (os.cpu_count() or 1))
spools = list(spool.split(size=size))
# this is a hack to get the progress bar to work. Essentially, we just
# add a secret flag to all but one spool so that progress bar is only
Expand Down Expand Up @@ -1010,15 +1012,19 @@ def maybe_mem_map(fid: IOBase, dtype="<u1") -> np.ndarray | np.memmap:
fid
A buffered reader, e.g. from open(file) as fid.
"""
try:
# File objects backed by memory (BytesIO and friends) have no
# usable name; those fall through to the in-memory read below.
raw = np.memmap(getattr(fid, "name", None), dtype=dtype, mode="r")
except (AttributeError, TypeError, ValueError):
# Fallback: read into memory
fid.seek(0)
raw = np.frombuffer(fid.read(), dtype=dtype)
return raw
# File objects backed by memory (BytesIO and friends) have no usable
# name, so there is nothing to map; they read into memory below.
name = getattr(fid, "name", None)
if name is not None:
try:
return np.memmap(name, dtype=dtype, mode="r")
except (AttributeError, OSError, TypeError, ValueError):
# A name which is not a mappable path -- an fd number, an empty
# file, a path already unlinked, a filesystem which cannot map --
# falls back rather than failing a read the handle can still do.
pass
fid.seek(0)
return np.frombuffer(fid.read(), dtype=dtype)
Comment thread
coderabbitai[bot] marked this conversation as resolved.


def deep_equality_check(obj1, obj2, visited=None):
Expand Down
25 changes: 23 additions & 2 deletions dascore/utils/patch.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import warnings
from collections import namedtuple
from collections.abc import Callable, Mapping, Sequence
from typing import Any, Literal, Protocol, cast
from typing import Any, Literal, Protocol, cast, overload

import numpy as np
import pandas as pd
Expand Down Expand Up @@ -303,7 +303,7 @@ def patch_function(
def _wrapper(func):
if validate_call:
config = dict(arbitrary_types_allowed=True)
func = pydantic.validate_call(func, config=config)
func = pydantic.validate_call(config=config)(func)

@functools.wraps(func)
def _func(patch, *args, **kwargs):
Expand Down Expand Up @@ -975,6 +975,25 @@ def _get_data_units_from_dims(patch, dims, operator):
return data_units


@overload
def _get_dx_or_spacing_and_axes(
patch,
dim,
require_sorted: bool = ...,
*,
require_evenly_spaced: Literal[True],
) -> tuple[tuple[float, ...], tuple[int, ...]]: ...


@overload
def _get_dx_or_spacing_and_axes(
patch,
dim,
require_sorted: bool = ...,
require_evenly_spaced: bool = ...,
) -> tuple[tuple[float | np.ndarray, ...], tuple[int, ...]]: ...


def _get_dx_or_spacing_and_axes(
patch,
dim,
Expand All @@ -994,6 +1013,8 @@ def _get_dx_or_spacing_and_axes(
If True, raise an error if all requested dimensions are not sorted.
require_evenly_spaced
If True, raise an error if all requested dimensions are not evenly sampled.
Every returned value is then a scalar spacing rather than an array of
values, which the overloads above make visible to callers.
"""
dims = iterate(dim if dim is not None else patch.dims)
out = []
Expand Down
18 changes: 8 additions & 10 deletions dascore/utils/time.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,9 @@ def _array_to_datetime64(array: np.ndarray) -> np.datetime64 | np.ndarray:
array = array.astype("datetime64[ns]")
# dealing with an array of datetime64 or empty array
if np.issubdtype(array.dtype, np.datetime64) or len(array) == 0:
if not array.shape: # dealing with degenerate (0-D( array
out = np.datetime64(array, "ns")
else:
out = array.astype("datetime64[ns]")
out = array.astype("datetime64[ns]")
if not array.shape: # unpack degenerate (0-D) array to a scalar
out = out[()]
# dealing with numerical data
elif np.issubdtype(array.dtype, np.timedelta64) or np.isreal(array[0]):
with np.errstate(divide="ignore", invalid="ignore"):
Expand Down Expand Up @@ -243,17 +242,16 @@ def _pass_time_delta(time_delta):
@to_timedelta64.register(np.ndarray)
@to_timedelta64.register(list)
@to_timedelta64.register(tuple)
def _array_to_timedelta64(array: np.ndarray) -> np.datetime64:
"""Convert an array of floating point timestamps to an array of np.datatime64."""
def _array_to_timedelta64(array: np.ndarray) -> np.timedelta64 | np.ndarray:
"""Convert an array of floating point durations to np.timedelta64."""
array = np.asarray(array)
# convert pure object arrays into float so sign casting works.
if np.issubdtype(array.dtype, np.dtype(object)):
array = array.astype(np.float64)
if np.issubdtype(array.dtype, np.timedelta64) or len(array) == 0:
if not array.shape: # unpack degenerate array
return np.timedelta64(array, "ns")
else:
return array.astype("timedelta64[ns]")
out = array.astype("timedelta64[ns]")
# unpack degenerate (0-D) array to a scalar
return out[()] if not array.shape else out
Comment thread
coderabbitai[bot] marked this conversation as resolved.
# Need to just get the ns form datetime64
elif np.issubdtype(array.dtype, np.datetime64):
int_array = array.view(np.int64)
Expand Down
7 changes: 2 additions & 5 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -260,14 +260,11 @@ line-ending = "lf"
include = ["dascore"] # tests/ still has many diagnostics; expand scope later.

# Rules with large pre-existing error counts, ignored until incrementally
# burned down. Counts as of 2026-08-03: invalid-argument-type 171,
# invalid-return-type 69, invalid-method-override 36, no-matching-overload 13,
# not-subscriptable 5.
# burned down. Counts as of 2026-08-04: invalid-argument-type 166,
# invalid-return-type 67, invalid-method-override 36.
[tool.ty.rules]
invalid-argument-type = "ignore"
invalid-return-type = "ignore"
no-matching-overload = "ignore"
not-subscriptable = "ignore"
invalid-method-override = "ignore"

# These files lazily import optional or untyped modules (xarray, numba,
Expand Down
7 changes: 7 additions & 0 deletions tests/test_core/test_coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -1687,6 +1687,13 @@ def test_non_coord_eq_self(self, basic_non_coord):
"""Ensure non coords are equal to themselves."""
assert basic_non_coord == basic_non_coord

def test_dimensionless_shape_survives_dump(self):
"""A partial coord keeps its shape when defaults are excluded."""
coord = get_coord(shape=())
dumped = coord.model_dump(exclude_defaults=True)
assert dumped["shape"] == ()
assert CoordPartial(**dumped) == coord

def test_empty_update_equal(self, basic_non_coord):
"""Empty update should produce an equal coord."""
out = basic_non_coord.update()
Expand Down
15 changes: 13 additions & 2 deletions tests/test_core/test_spool.py
Original file line number Diff line number Diff line change
Expand Up @@ -603,13 +603,24 @@ def test_yielded_spools_indexable(self, split_10):
patch = spool[0]
assert isinstance(patch, dc.Patch)

def test_spool_count(self, random_spool):
"""Ensure we can split based on desired size of spool."""
def test_uneven_size(self, random_spool):
"""Ensure a size which doesn't divide evenly leaves a short last spool."""
split = list(random_spool.split(size=2))
assert len(split) == 2
assert len(split[0]) == 2
assert len(split[1]) == 1

def test_non_integral_size(self, random_spool_len_10):
"""A size which isn't a whole number rounds up rather than raising."""
split = list(random_spool_len_10.split(size=2.5))
assert [len(x) for x in split] == [3, 3, 3, 1]

def test_spool_count(self, random_spool_len_10):
"""Ensure we can split based on the desired number of spools."""
split = list(random_spool_len_10.split(count=3))
assert len(split) == 3
assert sum(len(x) for x in split) == 10

def test_base_split_raises(self, random_spool):
"""Ensure BaseSpool split raises NoteImplementedError."""
msg = "has no split implementation"
Expand Down
20 changes: 20 additions & 0 deletions tests/test_utils/test_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,26 @@ def test_bytes_io(self):
assert isinstance(array, np.ndarray)
assert array.size == 4

def test_unmappable_file(self, tmp_path):
"""A named file numpy cannot map still reads into memory."""
path = tmp_path / "empty.bin"
path.touch()
with open(path, "rb") as fid:
array = maybe_mem_map(fid)
assert isinstance(array, np.ndarray)
assert not isinstance(array, np.memmap)
assert array.size == 0

def test_name_not_on_disk(self):
"""A handle whose name is not a real path still reads through it."""

class _NamedBytesIO(BytesIO):
name = "not-a-real-path"

array = maybe_mem_map(_NamedBytesIO(b"1234"))
assert not isinstance(array, np.memmap)
assert array.size == 4

def test_bytes_io_nonzero_position(self):
"""Fallback should read entire buffer even if pointer is not at 0."""
bio = BytesIO()
Expand Down
Loading