Skip to content

fix: two manifest validation gaps (single-list NaN handling, RowFilter operator) - #559

Merged
yakew7 merged 1 commit into
yakew7:mainfrom
propcgamer20-png:fix/manifest-validation-gaps
Sep 10, 2026
Merged

fix: two manifest validation gaps (single-list NaN handling, RowFilter operator)#559
yakew7 merged 1 commit into
yakew7:mainfrom
propcgamer20-png:fix/manifest-validation-gaps

Conversation

@propcgamer20-png

Copy link
Copy Markdown
Collaborator

#547 - disadvantaged_mask treats NaN as "known" in the single-list branches

ProtectedAttribute.disadvantaged_mask's branches for "only disadvantaged_values" or "only advantaged_values" given hardcoded known = pd.Series(True, index=df.index). A NaN in the column matches neither isin() nor its complement, so it was silently classified to one side instead of excluded - contradicting the module's own docstring and inconsistent with the both-lists branch, numeric_threshold, and age_interval_threshold, which all exclude unclassifiable rows via known_mask.

pa = ProtectedAttribute(name="race", type="categorical", column="race",
                        advantaged_values=["White", "Asian-Pac-Islander"])
mask, known = pa.disadvantaged_mask(pd.DataFrame({"race": ["White", "Black", None]}))
# before: disadv [F, T, T]   known [T, T, T]   <- NaN counted as disadvantaged
# after:  disadv [F, T, F]   known [T, T, F]   <- NaN excluded

Benefits Denial/audit.yaml uses the single-list pattern for race and national_origin, so a real missing value there would corrupt the fairness gap. Fix: known = col.notna() in both single-list branches.

#549 - RowFilter has no validation that an operator is set

Unlike TargetSpec and ProtectedAttribute (which validate required companion fields in __post_init__), RowFilter had none. A block with column set but no operator silently kept every row:

RowFilter(column="race").apply(df)   # returned df unchanged, no error

MANIFEST_SPEC.md documents "exactly one of the operators below" as required. Added __post_init__ raising ValueError when none of isin / not_isin / equals / not_equals / notna is set.

Tests

  • test_categorical_single_list_excludes_nan_via_known_mask (parametrized over both single-list branches)
  • test_row_filter_needs_at_least_one_operator

pytest tests/test_manifest.py -> all pass (33 -> 36), including test_every_shipped_manifest_loads_without_error (no shipped audit.yaml has an operator-less filter). ruff clean. The 3 tests/test_benchmark.py failures are pre-existing on main on this platform (manifest-error-wrapping path checks) and unrelated.

Closes #547
Closes #549

…r operator)

yakew7#547: ProtectedAttribute.disadvantaged_mask's single-list branches (only
disadvantaged_values OR only advantaged_values given) hardcoded
known_mask = True for every row, so a NaN in the column was not excluded
and got silently routed to whichever side isin() landed it on. The
both-lists branch, numeric_threshold, and age_interval_threshold all
correctly exclude unclassifiable rows via known_mask. Benefits Denial's
audit.yaml uses the single-list pattern for race and national_origin, so
a real missing value there would corrupt the fairness gap instead of
being dropped. known is now col.notna() in both single-list branches.

yakew7#549: RowFilter had no __post_init__, unlike its sibling dataclasses
TargetSpec and ProtectedAttribute. A row_filters entry with `column` set
but no operator (isin / not_isin / equals / not_equals / notna) silently
kept every row. MANIFEST_SPEC.md documents "exactly one of the operators
below" as required. Added __post_init__ raising ValueError when none is
set, matching the validation style already used in the sibling classes.

Closes yakew7#547
Closes yakew7#549
@vercel

vercel Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@propcgamer20-png is attempting to deploy a commit to the yashkewlani2020-gmailcom's projects Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

@yakew7 @ahmdkaml - new PR touching a path you own, please review.

@yakew7
yakew7 merged commit ad0135b into yakew7:main Sep 10, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants