From 1663840505652873f969ce00a448ca5ac8c92a3a Mon Sep 17 00:00:00 2001 From: Joshua Date: Mon, 24 Aug 2026 08:44:52 -0400 Subject: [PATCH] fix: reduce_factors NaN aggregation silently corrupted other observables (#119) reduce_factors aggregated per-factor importance across observables via np.maximum, which propagates NaN (np.maximum(4.05, nan) == nan). A single observable that's legitimately undefined in some regimes (e.g. a Type-I rate, only meaningful when the null hypothesis is true -- a completely reasonable, common Scorer pattern) silently erased a real, significant importance value found via a *different* observable for the same factor, dropping it from reduce_factors' output with no warning or error. Found in practice: yoavram-lab/pp-eigentest's Phase 1a screen had a factor (alpha) with a real, large Morris importance (mu*=4.05, far above threshold) on one observable, silently excluded because a different NaN-valued observable happened to appear later in the importance dict's iteration order. Worked around locally there (filtering NaN observables before the call); this is the actual fix. Uses np.fmax (NaN-safe) instead, seeded with NaN rather than zero so a factor with zero valid measurements across every observable stays NaN throughout rather than settling at a falsely-confident 0.0 -- and warns when that happens, since it reflects missing data, not confirmed unimportance. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 4 ++++ src/trade_study/design.py | 31 +++++++++++++++++++++++++-- tests/test_design.py | 45 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04a55c1..d413426 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/trade_study/design.py b/src/trade_study/design.py index cfc1111..0c7280e 100644 --- a/src/trade_study/design.py +++ b/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 diff --git a/tests/test_design.py b/tests/test_design.py index 4201958..67fcc90 100644 --- a/tests/test_design.py +++ b/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) # ---------------------------------------------------------------------------