Skip to content

_resolve_periodicity fails open, silently disabling xarray calendar conversion #759

Description

@monocongo

This was generated by AI during triage.

Summary

_resolve_periodicity in src/climate_indices/xarray_adapter.py (line 321) returns None whenever it cannot confidently determine a periodicity. Its caller treats None as "no calendar plan, no validation", so every uncertain case silently disables the mechanism whose entire purpose is to prevent silently wrong results.

This is not currently reachable — see below — but it is a fail-open default inside a safety check, and it is one signature change away from mattering.

Evidence

def _resolve_periodicity(func, modified_args, modified_kwargs, inferred_params):
    periodicity = inferred_params.get("periodicity")
    if periodicity is not None:
        return periodicity if isinstance(periodicity, compute.Periodicity) else None

    try:
        bound = inspect.signature(func).bind_partial(*modified_args, **modified_kwargs)
    except TypeError:
        return None

    periodicity = bound.arguments.get("periodicity")
    return periodicity if isinstance(periodicity, compute.Periodicity) else None

Three separate paths return None for reasons that are not "this function has no periodicity":

  1. inferred_params["periodicity"] is present but not a Periodicity instance
  2. bind_partial raises TypeError on an argument mismatch
  3. the bound periodicity argument is present but not a Periodicity instance

_resolve_daily_calendar_plan then returns None, and _compute_with_daily_calendar_plan runs the NumPy core against unconverted Gregorian values — the exact drift #757 removed.

Confirmed not exploitable today. A string periodicity such as spi(..., periodicity="daily") is rejected downstream by indices.spi with InvalidArgumentError before any result is returned, so no drifted output escapes to a caller. The protection is incidental to this function, not provided by it.

Current behavior

An unresolvable periodicity is indistinguishable from a function that legitimately has no periodicity parameter (pci, and any wrapper without one). Both produce None, and neither is logged.

Desired behavior

Distinguish "this function takes no periodicity" from "this function takes a periodicity and I could not resolve it". The first is normal and should stay quiet. The second should not silently skip calendar conversion — at minimum it should emit a structured log event, and preferably raise, since a wrapped function that declares a periodicity parameter whose value cannot be resolved indicates a programming error at the seam rather than a user input problem.

Follow the module's existing conventions: raise from the ClimateIndicesError hierarchy in exceptions.py, never a bare exception, and use get_logger() rather than stdlib logging. Add tests covering each of the three paths above.

Do not weaken the downstream InvalidArgumentError validation in indices.py — this issue is about the seam failing closed, not about relocating existing checks.

Context

Follow-up from the code review on #757. Flagged as advisory rather than blocking because it is unreachable through the current public API.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementepic:xarrayEpic 2 — xarray integration & notebooksready-for-agentFully specified and ready for an AFK agent

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions