fix: reduce_factors NaN aggregation silently corrupted other observables

CHANGELOG.md

@@ -14,6 +14,10 @@ All notable changes to this project will be documented in this file.
 - `recommend_bucketed_config()`: the discrete counterpart to `RegimeSurrogate` (#123). For a handful of named regimes too sparse (and too far outside any existing training data) for a surrogate to extrapolate across sensibly, runs `run_adaptive` independently per regime, picks each regime's best-found config by a primary objective, groups regimes into named buckets, and aggregates each bucket's per-regime best configs (median for continuous/discrete factors, mode for categorical) into one recommended config per bucket.
 - `recommend_bucketed_config()` split into `recommend_per_regime()` (the expensive adaptive search) and `aggregate_bucketed_config()` (cheap post-processing), with `recommend_bucketed_config()` now a thin wrapper composing them. Lets a caller experiment with different `bucket_fn` groupings against the same search results without re-running `run_adaptive` for every attempt -- found necessary in practice deriving VBPCApy buckets, where a first grouping choice performed poorly and needed re-grouping without repeating an hours-long search.
 
+### Fixed
+
+- `reduce_factors()` no longer lets a NaN-valued observable (e.g. a Type-I rate that's legitimately undefined outside null regimes) silently corrupt every other observable's importance for the same factor. It aggregated via `np.maximum`, which propagates NaN (`np.maximum(4.05, nan) == nan`); one conditionally-undefined observable could erase a real, significant importance value found via a *different* observable, dropping the factor with no warning or error. Now uses NaN-safe `np.fmax`, and warns when a factor's importance is NaN across *every* observable (dropped for lack of data, not confirmed unimportance) (#119).
+
 ## [0.2.0] — 2026-08-17
 
 ### Added

src/trade_study/design.py

@@ -6,6 +6,7 @@
 from __future__ import annotations
 
 import inspect
+import warnings
 from dataclasses import dataclass, field
 from enum import Enum
 from itertools import product
@@ -703,13 +704,39 @@ def reduce_factors(
 
     Returns:
         Reduced list of influential factors.
+
+    Warns:
+        UserWarning: If every observable's importance is NaN for one or
+            more factors (#119) -- those factors are dropped (there's no
+            valid data to compare against ``threshold``), but that
+            reflects missing data, not confirmed unimportance.
     """
     continuous = [f for f in factors if f.factor_type == FactorType.CONTINUOUS]
     non_continuous = [f for f in factors if f.factor_type != FactorType.CONTINUOUS]
 
-    max_importance = np.zeros(len(continuous))
+    # NaN-filled, not zero-filled: a factor with zero valid (non-NaN)
+    # measurements across every observable must stay NaN throughout, not
+    # silently settle at 0.0 -- indistinguishable from "confirmed
+    # unimportant" otherwise. np.fmax (not np.maximum) ignores NaN in
+    # either operand rather than propagating it, so one observable that's
+    # legitimately undefined in some regimes (e.g. a Type-I rate, NaN
+    # outside null regimes) can't erase a real, significant importance
+    # value a *different* observable found for the same factor (#119).
+    max_importance = np.full(len(continuous), np.nan)
     for arr in importance.values():
-        max_importance = np.maximum(max_importance, arr)
+        max_importance = np.fmax(max_importance, arr)
+
+    all_nan = np.isnan(max_importance)
+    if all_nan.any():
+        names = [continuous[i].name for i in np.flatnonzero(all_nan)]
+        warnings.warn(
+            f"reduce_factors: every observable's importance is NaN for "
+            f"{names} -- dropping them from the reduced list, but this "
+            f"reflects missing data, not confirmed unimportance. See "
+            f"https://github.com/jcm-sci/trade-study/issues/119.",
+            UserWarning,
+            stacklevel=2,
+        )
 
     kept = [
         f for f, imp in zip(continuous, max_importance, strict=True) if imp >= threshold

tests/test_design.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import importlib
+import warnings
 from typing import Any
 
 import numpy as np
@@ -600,6 +601,50 @@ def test_reduce_high_threshold_drops_all(continuous_factors: list[Factor]) -> No
     assert len(kept) == 0
 
 
+def test_reduce_nan_in_one_observable_does_not_erase_another(
+    continuous_factors: list[Factor],
+) -> None:
+    """A NaN-valued observable (#119) can't erase a real signal found elsewhere.
+
+    alpha has a real, large importance (0.5) on obs1; obs2 -- a
+    conditionally-undefined observable (e.g. a Type-I rate) -- is NaN for
+    every factor. Before #119's fix, np.maximum propagated that NaN and
+    silently dropped alpha despite obs1's real signal.
+    """
+    importance = {
+        "obs1": np.array([0.5, 0.01]),
+        "obs2": np.array([np.nan, np.nan]),
+    }
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        kept = reduce_factors(continuous_factors, importance, threshold=0.1)
+    names = [f.name for f in kept]
+    assert "alpha" in names
+    assert "beta" not in names
+
+
+def test_reduce_all_nan_for_factor_drops_and_warns(
+    continuous_factors: list[Factor],
+) -> None:
+    """A factor NaN across every observable is dropped, but with a warning."""
+    importance = {
+        "obs1": np.array([np.nan, 0.5]),
+        "obs2": np.array([np.nan, 0.01]),
+    }
+    with pytest.warns(UserWarning, match="alpha"):
+        kept = reduce_factors(continuous_factors, importance, threshold=0.1)
+    names = [f.name for f in kept]
+    assert "alpha" not in names
+    assert "beta" in names
+
+
+def test_reduce_no_nan_does_not_warn(continuous_factors: list[Factor]) -> None:
+    importance = {"y": np.array([0.5, 0.01])}
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        reduce_factors(continuous_factors, importance, threshold=0.1)
+
+
 # ---------------------------------------------------------------------------
 # FactorConstraint — coupled design-time constraints (#103)
 # ---------------------------------------------------------------------------