Merge pull request #6 from tjayasinghe/fix/release-gate-findings

Fix v1.0 release-gate review findings

Author: tjayasinghe

CHANGELOG.md

@@ -33,6 +33,19 @@ First public release.
 - Full Sphinx documentation (hosted on Read the Docs) and a reproducible validation +
   benchmark suite under `benchmarks/`.
 
+### Robustness
+
+- Peak selection returns the true peak even when it sits at a frequency-grid edge (a
+  signal whose period is comparable to the observing baseline), and never reports a
+  non-finite period or NaN-power sample.
+- Settings reject transposed frequency / period / duration-fraction bounds at
+  construction; the CLI's `--out` JSON is always standard JSON (no `NaN`/`Infinity`).
+- GPU kernels opt into larger shared memory where the device allows, and otherwise raise
+  a clear error naming the setting to reduce — instead of a raw CUDA driver error.
+- Batch sinks are correct across re-runs: a file sink is keyed by `(key, method)`, a
+  directory sink refuses a mismatched `chunk_size` on resume, and a CSV sink asked to
+  store raw spectra fails loudly rather than dropping the columns.
+
 ### Validated
 
 - Every method matches an established reference (astropy `LombScargle` / `BoxLeastSquares`,

src/cuperiod/batch/io.py

@@ -205,11 +205,15 @@ def _write_parquet(rows: Sequence[Mapping[str, Any]], path: Path) -> None:
 def _write_csv(rows: Sequence[Mapping[str, Any]], path: Path) -> None:
     import csv
 
-    columns = [
-        col
-        for col in _union_columns(rows)
-        if not any(isinstance(row.get(col), list) for row in rows)
+    columns = _union_columns(rows)
+    list_cols = [
+        col for col in columns if any(isinstance(row.get(col), list) for row in rows)
     ]
+    if list_cols:
+        raise ValueError(
+            f"CSV sink cannot store array-valued columns {list_cols} (e.g. from "
+            "store_raw); use a .parquet sink."
+        )
     tmp = path.with_suffix(path.suffix + ".tmp")
     with tmp.open("w", newline="", encoding="utf-8") as fh:
         writer = csv.DictWriter(fh, fieldnames=columns, extrasaction="ignore")

src/cuperiod/batch/runner.py

@@ -15,6 +15,7 @@
 
 from __future__ import annotations
 
+import json
 import warnings
 from collections.abc import Mapping, Sequence
 from concurrent.futures import ProcessPoolExecutor, as_completed
@@ -165,6 +166,29 @@ def _part_path(sink_dir: Path, idx: int) -> Path:
     return sink_dir / f"part-{idx:05d}.parquet"
 
 
+def _dir_manifest_guard(sink_dir: Path, chunk_size: int, resume: bool) -> None:
+    """Pin a directory sink's chunk_size so a resume cannot realign part indices.
+
+    Part files are named purely by chunk index, so resuming with a different chunk_size
+    would silently drop or duplicate light curves. Refuse the mismatch and record the
+    size for the next run.
+    """
+    manifest = sink_dir / "_manifest.json"
+    if resume and manifest.exists():
+        try:
+            prev = int(json.loads(manifest.read_text(encoding="utf-8"))["chunk_size"])
+        except Exception:  # noqa: BLE001 - a corrupt manifest must not abort the run
+            prev = chunk_size
+        if prev != chunk_size:
+            raise ValueError(
+                f"directory sink {sink_dir} was written with chunk_size={prev}; "
+                f"resuming requires the same chunk_size (got {chunk_size}). Use the "
+                "same chunk_size, a fresh directory, or resume=False."
+            )
+    sink_dir.mkdir(parents=True, exist_ok=True)
+    manifest.write_text(json.dumps({"chunk_size": chunk_size}), encoding="utf-8")
+
+
 def batch_periodograms(
     inputs: Any,
     method: str | Sequence[str] = "GLS",
@@ -232,6 +256,8 @@ def batch_periodograms(
 
     method_names = (method,) if isinstance(method, str) else tuple(method)
     methods = tuple(get_method(m).name for m in method_names)
+    if not methods:
+        raise ValueError("no methods specified")
     settings_map = {name: _settings_for(name, settings) for name in methods}
     cfg = _ChunkConfig(
         methods=methods,
@@ -243,11 +269,19 @@ def batch_periodograms(
         store_raw=store_raw,
     )
 
+    sink_kind, sink_dir, sink_file = _classify_sink(sink)
+    if store_raw and sink_kind == "file" and sink_file.suffix.lower() == ".csv":
+        raise ValueError(
+            "store_raw=True produces array-valued spectrum columns that a CSV sink "
+            "cannot hold; use a .parquet sink or a directory sink."
+        )
+
     items = resolve_inputs(
         inputs, columns=columns, domain=domain, band_column=band_column
     )
     chunks = _chunked(items, max(1, chunk_size))
-    sink_kind, sink_dir, sink_file = _classify_sink(sink)
+    if sink_kind == "dir":
+        _dir_manifest_guard(sink_dir, max(1, chunk_size), resume)
 
     pending = _pending_chunks(chunks, sink_kind, sink_dir, resume)
     n_skipped = len(items) - sum(len(chunks[i]) for i in pending)
@@ -322,8 +356,14 @@ def _classify_sink(sink: str | Path | None) -> tuple[str, Path, Path]:
     if sink is None:
         return "memory", Path(), Path()
     path = Path(sink)
-    if path.suffix.lower() in {".parquet", ".pq", ".csv"}:
+    suffix = path.suffix.lower()
+    if suffix in {".parquet", ".pq", ".csv"}:
         return "file", path.parent, path
+    if suffix and not path.is_dir():
+        raise ValueError(
+            f"unsupported sink {str(sink)!r}: a file sink must end in .parquet or "
+            ".csv; pass a directory (no extension) for a resumable multi-part sink."
+        )
     return "dir", path, path
 
 
@@ -376,8 +416,12 @@ def _run_pool(
 def _finalize_file(rows: list[dict[str, Any]], path: Path, resume: bool) -> None:
     if resume and path.exists():
         existing = _read_existing_rows(path)
-        seen = {r.get("key") for r in existing}
-        merged = existing + [r for r in rows if r.get("key") not in seen]
+        # Dedup on (key, method): a later run adding a different method to the same file
+        # must not be discarded as an already-seen key.
+        seen = {(r.get("key"), r.get("method")) for r in existing}
+        merged = existing + [
+            r for r in rows if (r.get("key"), r.get("method")) not in seen
+        ]
         write_rows(merged, path)
     else:
         write_rows(rows, path)

src/cuperiod/cli/app.py

@@ -13,7 +13,9 @@
 from __future__ import annotations
 
 import json
+import math
 from pathlib import Path
+from typing import Any
 
 import numpy as np
 import typer
@@ -47,7 +49,28 @@ def _parse_methods(method: str) -> list[str]:
 
 
 def _domain(value: str | None) -> Domain | None:
-    return None if value is None else Domain(value.lower())
+    if value is None:
+        return None
+    try:
+        return Domain(value.lower())
+    except ValueError as exc:
+        raise typer.BadParameter("domain must be 'magnitude' or 'flux'") from exc
+
+
+def _json_safe(obj: Any) -> Any:
+    """Recursively replace non-finite floats with ``None`` for standard JSON output.
+
+    ``json.dumps`` defaults to emitting bare ``Infinity``/``NaN`` tokens that most
+    non-Python JSON parsers reject. Peaks are finite by construction, but this sanitizes
+    the serialization boundary so a written ``--out`` file is always valid JSON.
+    """
+    if isinstance(obj, float):
+        return obj if math.isfinite(obj) else None
+    if isinstance(obj, dict):
+        return {k: _json_safe(v) for k, v in obj.items()}
+    if isinstance(obj, (list, tuple)):
+        return [_json_safe(v) for v in obj]
+    return obj
 
 
 @app.command()
@@ -81,7 +104,10 @@ def run(
             if isinstance(result, (MultiResult, Periodogram))
             else {}
         )
-        out.write_text(json.dumps(payload, indent=2), encoding="utf-8")
+        out.write_text(
+            json.dumps(_json_safe(payload), indent=2, allow_nan=False),
+            encoding="utf-8",
+        )
         typer.echo(f"\nWrote {out}")
     if save_periodogram is not None:
         arrays: dict[str, np.ndarray] = {}

src/cuperiod/core/backend.py

@@ -14,9 +14,15 @@
 import pathlib
 import sys
 from types import ModuleType
+from typing import Any
+
+from cuperiod.core.errors import BackendUnavailableError
 
 _CUDA_DLL_READY = False
 
+#: The default per-block dynamic shared-memory cap (bytes); larger needs an opt-in.
+_DEFAULT_SHARED_MEM = 48 * 1024
+
 
 def ensure_cuda_dll_path() -> None:
     """Put the ``nvidia-*-cu12`` wheel binary dirs on the Windows DLL search path.
@@ -85,6 +91,50 @@ def available_backends() -> set[str]:
     return out
 
 
+def ensure_shared_memory(
+    kernel: Any, dynamic_bytes: int, *, method: str, hint: str
+) -> None:
+    """Permit a cupy ``RawKernel`` to use ``dynamic_bytes`` of dynamic shared memory.
+
+    The per-block default cap is 48 KB; a larger request needs an explicit opt-in and is
+    still bounded by the device's ``MaxSharedMemoryPerBlockOptin``. When the request
+    exceeds what the device can provide, raise a clear :class:`BackendUnavailableError`
+    (pointing at the setting to reduce) instead of letting the kernel launch fail with a
+    raw ``CUDADriverError``.
+
+    Parameters
+    ----------
+    kernel : cupy.RawKernel
+        The kernel about to be launched.
+    dynamic_bytes : int
+        The ``shared_mem=`` size the launch will request.
+    method : str
+        Method name, for the error message.
+    hint : str
+        The setting(s) the user should reduce, for the error message.
+    """
+    import cupy
+
+    optin = int(
+        cupy.cuda.Device().attributes.get(
+            "MaxSharedMemoryPerBlockOptin", _DEFAULT_SHARED_MEM
+        )
+    )
+    try:
+        static = int(kernel.attributes.get("shared_size_bytes", 0))
+    except Exception:
+        static = 0
+    needed = int(dynamic_bytes) + static
+    if needed > optin:
+        raise BackendUnavailableError(
+            f"{method}: needs ~{needed // 1024} KB of GPU shared memory per block but "
+            f"the device allows at most {optin // 1024} KB; reduce {hint}, or use "
+            "backend='cpu'."
+        )
+    if int(dynamic_bytes) > _DEFAULT_SHARED_MEM:
+        kernel.max_dynamic_shared_size_bytes = int(dynamic_bytes)
+
+
 def array_module(a: object) -> ModuleType:
     """Return cupy for a device array, else numpy (for device-agnostic assembly)."""
     try:
@@ -103,5 +153,6 @@ def array_module(a: object) -> ModuleType:
     "available_backends",
     "cuda_available",
     "ensure_cuda_dll_path",
+    "ensure_shared_memory",
     "has_module",
 ]

src/cuperiod/core/config.py

@@ -11,20 +11,34 @@
 
 from __future__ import annotations
 
-from typing import Literal
+from typing import Literal, Self
 
-from pydantic import Field
+from pydantic import Field, model_validator
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
 #: Backend selector accepted by every method (concrete names are method-specific).
 BackendName = str
 
 
+def _require_lt(lo: float | None, hi: float | None, lo_name: str, hi_name: str) -> None:
+    """Raise ``ValueError`` if both bounds are set and ``lo`` is not below ``hi``."""
+    if lo is not None and hi is not None and lo >= hi:
+        raise ValueError(f"{lo_name} ({lo}) must be < {hi_name} ({hi})")
+
+
 class GLSSettings(BaseSettings):
     """Settings for the generalized Lomb-Scargle (GLS) periodogram."""
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_GLS_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.minimum_frequency, self.maximum_frequency,
+            "minimum_frequency", "maximum_frequency",
+        )
+        return self
+
     minimum_frequency: float | None = Field(
         default=None,
         description="Lowest trial frequency (cycles/day); None -> 1/baseline.",
@@ -68,6 +82,18 @@ class BLSSettings(BaseSettings):
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_BLS_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.min_period_days, self.max_period_days,
+            "min_period_days", "max_period_days",
+        )
+        _require_lt(
+            self.duration_min_frac, self.duration_max_frac,
+            "duration_min_frac", "duration_max_frac",
+        )
+        return self
+
     min_period_days: float = Field(default=0.2, gt=0.0, description="Minimum period.")
     max_period_days: float = Field(default=100.0, gt=0.0, description="Maximum period.")
     min_transits: int = Field(
@@ -129,6 +155,14 @@ class PDMSettings(BaseSettings):
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_PDM_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.minimum_frequency, self.maximum_frequency,
+            "minimum_frequency", "maximum_frequency",
+        )
+        return self
+
     minimum_frequency: float | None = Field(
         default=None,
         description="Lowest trial frequency (cycles/day); None -> 1/baseline.",
@@ -170,6 +204,14 @@ class MHAOVSettings(BaseSettings):
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_MHAOV_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.minimum_frequency, self.maximum_frequency,
+            "minimum_frequency", "maximum_frequency",
+        )
+        return self
+
     minimum_frequency: float | None = Field(
         default=None,
         description="Lowest trial frequency (cycles/day); None -> 1/baseline.",
@@ -210,6 +252,14 @@ class CESettings(BaseSettings):
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_CE_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.minimum_frequency, self.maximum_frequency,
+            "minimum_frequency", "maximum_frequency",
+        )
+        return self
+
     minimum_frequency: float | None = Field(
         default=None,
         description="Lowest trial frequency (cycles/day); None -> 1/baseline.",
@@ -249,6 +299,14 @@ class StringLengthSettings(BaseSettings):
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_SL_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.minimum_frequency, self.maximum_frequency,
+            "minimum_frequency", "maximum_frequency",
+        )
+        return self
+
     minimum_frequency: float | None = Field(
         default=None,
         description="Lowest trial frequency (cycles/day); None -> 1/baseline.",
@@ -286,6 +344,18 @@ class TLSSettings(BaseSettings):
 
     model_config = SettingsConfigDict(env_prefix="CUPERIOD_TLS_", extra="forbid")
 
+    @model_validator(mode="after")
+    def _check_bounds(self) -> Self:
+        _require_lt(
+            self.min_period_days, self.max_period_days,
+            "min_period_days", "max_period_days",
+        )
+        _require_lt(
+            self.duration_min_frac, self.duration_max_frac,
+            "duration_min_frac", "duration_max_frac",
+        )
+        return self
+
     min_period_days: float = Field(default=0.5, gt=0.0, description="Minimum period.")
     max_period_days: float = Field(default=100.0, gt=0.0, description="Maximum period.")
     min_transits: int = Field(