From f6f4db53483bba3e06784f7264856fc7755a1b53 Mon Sep 17 00:00:00 2001 From: Chris Nicholas <4948774+cnicholas@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:48:54 -0400 Subject: [PATCH] fix(data-preparation): a missing factor value no longer makes a multi-factor study fail ## Summary - **What:** rows with no value in any factor column leave the analysis before the composite subgroup label is built, for one factor or many, with a ProcessBehaviorWarning naming the count per factor. - **Why:** with one factor such rows were dropped silently by the later rsg dropna; with two or more, _add_composite_column validated before that drop and raised "Cannot build RSG ... missing values in factor columns". The same file formulated with either factor alone and failed with both (Tom's survey file: 85,101 rows, 7,980 blank SURVEY QUESTION). - **Scope:** data_preparation.py (one helper, called before _add_grouping_column); tests/test_na_in_factor_columns.py; CHANGELOG. ## Contract / Invariants (must remain true) - Files with complete factor columns: byte-identical results, no warning (pinned). - Single-factor studies drop exactly the rows they dropped before; the composite check in _add_composite_column stays as a safety net. - ODS detection on raw data is unchanged (its own detector already filters NA factor rows; pinned by test_na_in_factor_is_filtered). - No chart math; validation/e2e_bishop_report.py exit 0. ## Behavior Changes (explicit) - formulate() with >=2 factors and a blank factor value now succeeds, dropping those rows, instead of raising. - One ProcessBehaviorWarning per formulate() when any factor value is missing: "Dropped 7,980 of 85,101 rows with no value in a factor column: SURVEY QUESTION (7,980 missing). ..." ## Tests - two factors with blanks formulate; one- and two-factor paths drop the same rows and create no 'nan' level; the warning carries per-column counts and names only columns with gaps; no warning when complete; blanks in both factors are named per column. ## Manual Verification - pytest tests/: 2365 passed, 10 skipped; ruff clean; mypy 17 pre-existing - validation/e2e_bishop_report.py: exit 0 - Tom's survey file, both factors: ADS 1, K 20, T 12, 77,121 rows, Xbar/S execute --- CHANGELOG.md | 9 +++ processbehavior/data_preparation.py | 34 +++++++++- tests/test_na_in_factor_columns.py | 96 +++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 tests/test_na_in_factor_columns.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 560e99f..95c44a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 No arithmetic changed; none of these names is exported from the package top level. ### Fixed +- **A missing value in a factor column no longer makes a two-factor study fail.** With one + factor, rows with a blank factor value were dropped silently; with two or more, the + composite subgroup label was built before that drop and ``formulate`` raised + ``ValidationError: Cannot build RSG ... missing values in factor columns``. The same + file formulated with either factor alone and failed with both (a 85,101-row survey + file with 7,980 blank ``SURVEY QUESTION`` rows). Rows with no value in any factor + column now leave the analysis before the label is built, for any number of factors, + and the library says so with a ``ProcessBehaviorWarning`` naming the count per + factor. Response and time missing values are handled as before. - **``evaluate`` no longer raises on a constant column.** Binning a column with no spread (every value identical) returned the documented no-spread result for ``equal_freq`` but raised ``ValueError: Bin edges must be unique`` from ``pd.cut`` for ``equal_width`` and diff --git a/processbehavior/data_preparation.py b/processbehavior/data_preparation.py index 29c0718..0e3db11 100644 --- a/processbehavior/data_preparation.py +++ b/processbehavior/data_preparation.py @@ -16,6 +16,7 @@ from __future__ import annotations import logging +import warnings from functools import reduce from typing import TYPE_CHECKING @@ -24,7 +25,7 @@ from natsort import natsorted from pandas.api.types import is_numeric_dtype -from .exceptions import ColumnNotFoundError, FactorNotFoundError, ValidationError +from .exceptions import ColumnNotFoundError, FactorNotFoundError, ProcessBehaviorWarning, ValidationError if TYPE_CHECKING: from .formulation_spec import FormulationSpec @@ -281,6 +282,13 @@ def prepare_dataset(self, df: pd.DataFrame, spec: FormulationSpec) -> pd.DataFra # Add composite grouping variable if needed if spec.has_grouping: + # A row with no value in a factor column belongs to no cell, so it leaves the + # analysis here — for one factor or many. The single-factor path used to drop + # such rows silently via the rsg dropna below; the multi-factor path raised + # from _add_composite_column before reaching it, so the same file formulated + # with one factor and failed with two (Tom's survey file, 7,980 blank + # SURVEY QUESTION rows). Both paths now drop first and say so. + out = self._drop_rows_missing_factor_values(out, spec) out = self._add_grouping_column(out, spec) # Drop rows with missing values in analysis-critical columns BEFORE @@ -533,6 +541,30 @@ def build_keys(self, df: pd.DataFrame, spec: FormulationSpec) -> pd.DataFrame: # Private Helper Methods # ======================================================================== + def _drop_rows_missing_factor_values(self, df: pd.DataFrame, spec: FormulationSpec) -> pd.DataFrame: + """ + Drop rows with a missing value in any factor column, and report it. + + Reported as a ``ProcessBehaviorWarning`` (the house style for "the + library changed your data on your behalf"), naming the count per + factor, so an analyst sees that 7,980 rows left the study rather than + discovering a smaller N later. + """ + factor_cols = spec.rsg_vars_list + missing = df[factor_cols].isna() + if not missing.any().any(): + return df + per_col = {col: int(n) for col, n in missing.sum().items() if n} + n_rows = int(missing.any(axis=1).sum()) + warnings.warn( + f'Dropped {n_rows:,} of {len(df):,} rows with no value in a factor column: ' + + ', '.join(f'{col} ({n:,} missing)' for col, n in per_col.items()) + + '. A row with no factor value belongs to no cell, so it is excluded from the analysis.', + ProcessBehaviorWarning, + stacklevel=6, + ) + return df.loc[~missing.any(axis=1)] + def _add_grouping_column(self, df: pd.DataFrame, spec: FormulationSpec) -> pd.DataFrame: """ Add composite grouping column (e.g., 'lane_head'). diff --git a/tests/test_na_in_factor_columns.py b/tests/test_na_in_factor_columns.py new file mode 100644 index 0000000..c85b02f --- /dev/null +++ b/tests/test_na_in_factor_columns.py @@ -0,0 +1,96 @@ +"""Rows with a missing factor value leave the study, for one factor or many, with a warning. + +Found on a survey file: 85,101 rows, 7,980 with a blank SURVEY QUESTION. It formulated with +either factor alone and raised with both, because the two-factor path built the composite +subgroup label before the missing-value drop. Nothing in the app surfaced that for long +enough to read. +""" + +import warnings + +import numpy as np +import pandas as pd +import pytest + +import processbehavior as pb +from processbehavior import ProcessBehaviorWarning + + +@pytest.fixture +def survey(): + """Two hospitals x 10 questions x 12 months, 6 replicates, with 8% of question labels blank. + + Six replicates so that no cell falls below two after the blanks leave: the study stays + at full replication and the test checks the drop, not a change of design state. + """ + rng = np.random.default_rng(11) + rows = [] + for month in range(1, 13): + for hospital in ('A', 'B'): + for q in ('LR', 'NC', 'NI', 'NP', 'PC', 'PI', 'PT', 'QF', 'SA', 'SD'): + for _ in range(6): + rows.append( + {'MONTH': month, 'HOSPITAL': hospital, 'QUESTION': q, 'SCORE': rng.choice([0, 25, 50, 75, 100])} + ) + df = pd.DataFrame(rows) + blank = rng.random(len(df)) < 0.08 + df.loc[blank, 'QUESTION'] = np.nan + return df, int(blank.sum()) + + +def _formulate(df, factors): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + st = pb.formulate(df, response='SCORE', factors=factors, time='MONTH') + return st, [w for w in caught if issubclass(w.category, ProcessBehaviorWarning)] + + +def test_two_factors_formulate_with_missing_factor_values(survey): + df, n_blank = survey + st, pbw = _formulate(df, ['QUESTION', 'HOSPITAL']) + assert st.analytical_design_state.sds == 1 + assert len(st._ads.analysis_dataset) == len(df) - n_blank + assert st.design().K == 20 # 10 questions x 2 hospitals, no phantom 'nan' level + + +def test_one_factor_and_two_factors_drop_the_same_rows(survey): + df, n_blank = survey + one, _ = _formulate(df, ['QUESTION']) + two, _ = _formulate(df, ['QUESTION', 'HOSPITAL']) + assert len(one._ads.analysis_dataset) == len(two._ads.analysis_dataset) == len(df) - n_blank + assert 'nan' not in set(one._ads.analysis_dataset['rsg'].astype(str)) + + +def test_the_drop_is_reported_with_counts(survey): + df, n_blank = survey + _, pbw = _formulate(df, ['QUESTION', 'HOSPITAL']) + texts = [str(w.message) for w in pbw] + hit = [t for t in texts if 'no value in a factor column' in t] + assert hit, texts + assert f'Dropped {n_blank:,} of {len(df):,} rows' in hit[0] + assert f'QUESTION ({n_blank:,} missing)' in hit[0] + assert 'HOSPITAL' not in hit[0] # only the columns that actually had gaps are named + + +def test_no_warning_when_factor_columns_are_complete(survey): + df, _ = survey + complete = df.dropna(subset=['QUESTION']) + _, pbw = _formulate(complete, ['QUESTION', 'HOSPITAL']) + assert not [w for w in pbw if 'factor column' in str(w.message)] + + +def test_missing_values_in_both_factors_are_named_per_column(): + df = pd.DataFrame( + { + 'lane': ['A', 'A', None, 'B', 'B', 'B'], + 'head': [1, 1, 1, None, 2, 2], + 't': [1, 1, 1, 1, 1, 1], + 'y': [1.0, 2.0, 3.0, 4.0, 5.0, 6.0], + } + ) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + st = pb.formulate(df, response='y', factors=['lane', 'head'], time='t') + msg = next(str(w.message) for w in caught if 'factor column' in str(w.message)) + assert 'Dropped 2 of 6 rows' in msg and 'lane (1 missing)' in msg and 'head (1 missing)' in msg + assert len(st._ads.analysis_dataset) == 4