diff --git a/faircode/manifest.py b/faircode/manifest.py index 9611601..8300e43 100644 --- a/faircode/manifest.py +++ b/faircode/manifest.py @@ -29,6 +29,17 @@ class RowFilter: not_equals: object | None = None notna: bool = False + def __post_init__(self): + # MANIFEST_SPEC.md requires "exactly one of the operators below" for a + # row_filters entry. A block with only `column` set (a blanked or + # copy-pasted YAML entry) would otherwise silently keep every row. + if (self.isin is None and self.not_isin is None and self.equals is None + and self.not_equals is None and not self.notna): + raise ValueError( + f"{self.column}: row filter needs one of " + f"isin / not_isin / equals / not_equals / notna" + ) + def apply(self, df: pd.DataFrame) -> pd.DataFrame: mask = pd.Series(True, index=df.index) if self.isin is not None: @@ -121,11 +132,16 @@ def disadvantaged_mask(self, df: pd.DataFrame) -> tuple[pd.Series, pd.Series]: known = col.isin(self.disadvantaged_values) | col.isin(self.advantaged_values) disadv = col.isin(self.disadvantaged_values) elif self.disadvantaged_values is not None: - known = pd.Series(True, index=df.index) + # Only one list given: a NaN can't be classified either way + # (it matches neither isin() nor its complement), so exclude + # it via known_mask - matching the both-lists branch above + # and every other attribute type below, instead of silently + # routing it to the advantaged side. + known = col.notna() disadv = col.isin(self.disadvantaged_values) elif self.advantaged_values is not None: - known = pd.Series(True, index=df.index) - disadv = ~col.isin(self.advantaged_values) + known = col.notna() + disadv = ~col.isin(self.advantaged_values) & known else: raise ValueError(f"{self.name}: need disadvantaged_values or advantaged_values") return disadv, known diff --git a/tests/test_manifest.py b/tests/test_manifest.py index a53f209..09dd169 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -202,6 +202,35 @@ def test_protected_attribute_categorical_complement(): assert known.all() +@pytest.mark.parametrize("kwargs", [ + {"advantaged_values": ["white"]}, + {"disadvantaged_values": ["black"]}, +]) +def test_categorical_single_list_excludes_nan_via_known_mask(kwargs): + # A NaN can't be classified when only one list is given, so known_mask + # must be False for it - previously the single-list branches hardcoded + # known=True and silently routed NaN to whichever side isin() landed it + # on (#547). + pa = ProtectedAttribute(name="g", type="categorical", column="g", **kwargs) + df = pd.DataFrame({"g": ["white", "black", None]}) + + disadv, known = pa.disadvantaged_mask(df) + + assert known.tolist() == [True, True, False] + assert not bool(disadv.iloc[2]) # the NaN row is not counted as disadvantaged + + +def test_row_filter_needs_at_least_one_operator(): + # MANIFEST_SPEC.md requires exactly one operator; a bare column silently + # kept every row instead of raising (#549). + with pytest.raises(ValueError, match="row filter needs one of"): + RowFilter(column="race") + + # notna=True counts as an operator; so does any of the others. + assert RowFilter(column="race", notna=True).column == "race" + assert RowFilter(column="race", equals="X").column == "race" + + def test_protected_attribute_numeric_threshold(): pa = ProtectedAttribute(name="age", type="numeric_threshold", column="age", threshold=30, disadvantaged="below")