Fix silent column collisions and accounting-style negatives in ingest

CHANGELOG.md

@@ -88,6 +88,13 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
   probabilities from cross-validated folds.
 
 ### Fixed
+- `philanthropy.ingest.map_columns` now raises a `ValueError` naming the
+  colliding source columns when a mapping sends two different source columns
+  to the same target name, instead of silently producing a duplicate-named
+  output column that could still pass a `required=` check.
+- `philanthropy.ingest._civicrm._to_amount` (shared by the CiviCRM, Raiser's
+  Edge and NPSP readers) now treats an accounting-style parenthesised amount
+  like `"($50.00)"` as negative instead of dropping the sign.
 - `MajorGiftClassifier.predict_affinity_score` raised `IndexError` after a
   fit on single-class labels, because it indexed `predict_proba(X)[:, 1]`
   unconditionally. It now mirrors the single-class guard already used by

philanthropy/ingest/_civicrm.py

@@ -359,7 +359,12 @@ def _to_amount(series: pd.Series) -> pd.Series:
     """
     if pd.api.types.is_numeric_dtype(series):
         return pd.to_numeric(series, errors="coerce")
-    cleaned = series.astype("string").str.replace(r"[^\d.\-]", "", regex=True)
+    # Accounting notation wraps a negative in parens, e.g. "($50.00)"; turn it
+    # into a leading minus sign before stripping everything else, or it reads
+    # as a plain positive.
+    text = series.astype("string").str.strip()
+    text = text.str.replace(r"^\((.*)\)$", r"-\1", regex=True)
+    cleaned = text.str.replace(r"[^\d.\-]", "", regex=True)
     return pd.to_numeric(cleaned, errors="coerce")
 
 

philanthropy/ingest/_map_columns.py

@@ -70,7 +70,21 @@ def map_columns(
         ...
     ValueError: missing required column(s) after mapping: amount
     """
-    renamed = df.rename(columns=dict(mapping))
+    mapping = dict(mapping)
+    targets: "dict[str, list[str]]" = {}
+    for source, target in mapping.items():
+        if source in df.columns:
+            targets.setdefault(target, []).append(source)
+    collisions = {target: sources for target, sources in targets.items() if len(sources) > 1}
+    if collisions:
+        detail = "; ".join(
+            f"{target!r} <- {sources}" for target, sources in collisions.items()
+        )
+        raise ValueError(
+            "mapping assigns multiple source columns to the same target: " + detail
+        )
+
+    renamed = df.rename(columns=mapping)
     missing = [col for col in required if col not in renamed.columns]
     if missing:
         raise ValueError(

tests/test_civicrm.py

@@ -561,3 +561,23 @@ def test_features_feed_donor_propensity_model():
     scores = model.predict_affinity_score(feats[cols].to_numpy())
     assert scores.shape == (len(feats),)
     assert np.isfinite(scores).all()
+
+
+# --------------------------------------------------------------------------- #
+# _to_amount (shared by CiviCRM, Raiser's Edge and NPSP readers)
+# --------------------------------------------------------------------------- #
+@pytest.mark.parametrize(
+    "raw, expected",
+    [
+        ("($50.00)", -50.0),
+        ("(50)", -50.0),
+        ("-25.50", -25.50),
+        ("$1,000.00", 1000.0),
+    ],
+)
+def test_to_amount_parses_signed_and_formatted_values(raw, expected):
+    from philanthropy.ingest._civicrm import _to_amount
+
+    out = _to_amount(pd.Series([raw]))
+
+    assert out.iloc[0] == pytest.approx(expected)

tests/test_map_columns.py

@@ -62,3 +62,10 @@ def test_does_not_mutate_input_dataframe():
     map_columns(df, {"CnID": "contact_id"})
 
     assert list(df.columns) == ["CnID"]
+
+
+def test_raises_on_collision_naming_the_colliding_sources():
+    df = pd.DataFrame({"A": [1], "B": [2]})
+
+    with pytest.raises(ValueError, match="A.*B|B.*A"):
+        map_columns(df, {"A": "x", "B": "x"}, required=["x"])