fix(currency): address code review feedback on accounting negatives, identifier roles, and ambiguity handling

Author: muhammad-muneeb3

src/freshdata/engine/context.py

@@ -139,9 +139,7 @@ def infer_role(
     label = str(name)
     if _is_target_name(label, config):
         return "target"
-    if label in config.id_columns or (
-        _ID_NAME.search(label) and not _MONEY_NAME.search(label)
-    ):
+    if label in config.id_columns or _ID_NAME.search(label):
         return "id"
     if is_datetime64_any_dtype(s):
         return "datetime"

src/freshdata/semantic/context.py

@@ -81,6 +81,7 @@ def _build_info(
     currencies: tuple[str, ...] = (),
     n_nonnull_override: int | None = None,
 ) -> SemanticColumnInfo:
+    """Build column semantic metadata from engine role and value distributions."""
     name = str(col)
     series = df[col]
     # On the native distinct path *series* holds only distinct values, so the

src/freshdata/semantic/experts.py

@@ -113,6 +113,7 @@ def parse_number_words(text: str) -> int | None:
 #: consults this table. Deliberately small: a currency absent from it and not
 #: settled by structure is reported ambiguous rather than guessed.
 _COMMA_DECIMAL_CURRENCIES = frozenset({"EUR", "CHF"})
+_ACCOUNTING_NUM_RE = re.compile(r"^[+-]?\s*\d[\d\s.,']*$")
 
 
 def _valid_grouping(part: str, sep: str) -> bool:
@@ -215,22 +216,33 @@ def _split_amount(body: str, code: str | None) -> tuple[float | None, bool]:
 def parse_currency_parts(text: str) -> tuple[float | None, bool]:
     """``(value, ambiguous)`` for a currency string.
 
-    Requires an explicit currency marker (symbol or ISO-ish code) so that a bare
-    ``"1,200"`` is left to ordinary dtype repair, not treated as money.
+    Requires an explicit currency marker (symbol or ISO-ish code), or an
+    accounting-negative parenthesized amount with valid accounting punctuation,
+    so that bare numbers and unit strings are left to ordinary repair.
     """
     s = text.strip()
     accounting_negative = s.startswith("(") and s.endswith(")")
     if accounting_negative:
         s = s[1:-1].strip()
-    has_symbol = any(c in s for c in _CURRENCY_SYMBOLS)
+    if not s:
+        return None, False
     codes = {t.lower() for t in re.findall(r"[A-Za-z]+", s)}
+    if codes - _CURRENCY_CODES:
+        return None, False
+    has_symbol = any(c in s for c in _CURRENCY_SYMBOLS)
     has_code = bool(codes & _CURRENCY_CODES)
     if not (has_symbol or has_code or accounting_negative):
         return None, False
+    if (
+        accounting_negative
+        and not (has_symbol or has_code)
+        and (not ("." in s or "," in s) or not _ACCOUNTING_NUM_RE.match(s))
+    ):
+        return None, False
     body = re.sub(r"[A-Za-z$€£¥₹\s\u00a0\u202f']", "", s)
     value, ambiguous = _split_amount(body, detect_currency(s))
     if value is not None and accounting_negative:
-        value = -value
+        value = -abs(value) if value != 0 else 0.0
     return value, ambiguous
 
 
@@ -603,14 +615,16 @@ class CurrencyStringExpert:
     issue_type = "currency_string"
 
     def applies(self, info: SemanticColumnInfo) -> bool:
+        """True when the column has monetary semantic characteristics and is not free text."""
         return info.money_like and not info.free_text
 
     def propose(self, series: pd.Series, info: SemanticColumnInfo) -> list[SemanticProposal]:
+        """Propose numeric conversions for formatted currency strings."""
         out: list[SemanticProposal] = []
         for raw, count in _value_counts(series).items():
             if not isinstance(raw, str):
                 continue
-            value = parse_currency(raw)
+            value, ambiguous = parse_currency_parts(raw)
             if value is None:
                 continue
             code = detect_currency(raw)
@@ -651,6 +665,36 @@ def propose(self, series: pd.Series, info: SemanticColumnInfo) -> list[SemanticP
                     )
                 )
                 continue
+            if ambiguous:
+                out.append(
+                    make_proposal(
+                        column=info.name,
+                        raw_value=raw,
+                        proposed_value=value,
+                        issue_type=self.issue_type,
+                        expert=self.name,
+                        base_confidence=0.60,
+                        evidence=(
+                            SemanticEvidence(
+                                "pattern", f"{raw!r} is an ambiguous currency amount", 0.0
+                            ),
+                            SemanticEvidence(
+                                "context_hint",
+                                f"{raw!r} has ambiguous thousands/decimal punctuation; "
+                                "needs human review",
+                                0.0,
+                            ),
+                        ),
+                        count=int(count),
+                        rationale=(
+                            f"{raw!r} is ambiguous between thousands and decimal separator; "
+                            "needs human review"
+                        ),
+                        info=info,
+                        risk_override="high",
+                    )
+                )
+                continue
             evidence = (
                 SemanticEvidence("pattern", f"{raw!r} is a currency string", 0.0),
                 SemanticEvidence("column_role", "column reads as monetary", 0.02),

tests/test_currency_locale.py

@@ -18,7 +18,14 @@
 import pytest
 
 import freshdata as fd
-from freshdata.semantic.experts import parse_currency, parse_currency_parts
+from freshdata.config import CleanConfig
+from freshdata.engine.context import infer_role
+from freshdata.semantic.experts import (
+    CurrencyStringExpert,
+    parse_currency,
+    parse_currency_parts,
+)
+from freshdata.semantic.types import SemanticColumnInfo
 
 FIXTURES_DIR = Path(__file__).parent / "fixtures"
 
@@ -121,6 +128,7 @@ def test_clean_does_not_scale_european_amounts(text, expected):
 
 
 def test_clean_financial_ledger_fixture_respects_locale_and_accounting_values():
+    """Verify financial ledger cleaning preserves row counts, dtypes, and values."""
     fixture = pd.read_csv(FIXTURES_DIR / "financial_ledger.csv")
     expectations = json.loads(
         (FIXTURES_DIR / "financial_ledger.expectations.json").read_text()
@@ -137,3 +145,102 @@ def test_clean_financial_ledger_fixture_respects_locale_and_accounting_values():
         actual = cleaned.loc[cleaned["transaction_id"] == transaction_id, "amount"]
         assert len(actual) == 1
         assert actual.iloc[0] == expected
+
+
+# -- PR #503 review fixes verification --------------------------------------
+
+
+@pytest.mark.parametrize(
+    ("text", "expected"),
+    [
+        ("(-$1,250.00)", -1250.0),
+        ("($-1,250.00)", -1250.0),
+        ("(-EUR 500.00)", -500.0),
+        ("(-10.50)", -10.50),
+    ],
+)
+def test_accounting_negatives_preserve_existing_negative_sign(text, expected):
+    """An explicit minus sign inside accounting parentheses must not flip positive."""
+    val, _ = parse_currency_parts(text)
+    assert val == expected
+
+
+@pytest.mark.parametrize(
+    "text",
+    [
+        "(10 kg)",
+        "(page 3)",
+        "(see item 42)",
+        "(10)",
+        "(100)",
+        "(10%)",
+        "(10 / 20)",
+        "( - )",
+    ],
+)
+def test_unit_and_word_strings_in_parentheses_are_not_currency(text):
+    """Parenthesized units and citations must not be treated as negative currency."""
+    val, ambiguous = parse_currency_parts(text)
+    assert val is None
+    assert ambiguous is False
+
+
+def test_unmarked_parenthetical_with_ambiguous_separators_flagged():
+    """Unmarked numbers like (1,250) must be flagged ambiguous and routed to review."""
+    val, ambiguous = parse_currency_parts("(1,250)")
+    assert val == -1250.0
+    assert ambiguous is True
+
+    val2, ambiguous2 = parse_currency_parts("(1.250)")
+    assert val2 == -1.25
+    assert ambiguous2 is True
+
+    expert = CurrencyStringExpert()
+    info = SemanticColumnInfo(
+        name="amount",
+        role="numeric",
+        n_nonnull=1,
+        nunique=1,
+        high_cardinality=False,
+        preserve=False,
+        free_text=False,
+        numeric_like=True,
+        boolean_like=False,
+        money_like=True,
+        unit_like=False,
+        identifier_like=False,
+    )
+    series = pd.Series(["(1,250)"])
+    proposals = expert.propose(series, info)
+    assert len(proposals) == 1
+    assert proposals[0].risk == "high"
+    assert proposals[0].confidence <= 0.60
+
+
+def test_payment_id_retains_identifier_role():
+    """Names matching _ID_NAME must retain id role even when matching _MONEY_NAME."""
+    cfg = CleanConfig()
+    # Repeating values (nunique != non_null) ensure role is not inferred purely by cardinality
+    for name in ("payment_id", "charge_id", "fee_id", "payment_key", "balance_uuid"):
+        series = pd.Series(["ID1", "ID1", "ID2"])
+        assert infer_role(name, series, cfg) == "id"
+
+
+def test_free_text_monetary_columns_stay_protected():
+    """Free-text columns marked money_like must stay protected from currency conversion."""
+    expert = CurrencyStringExpert()
+    info = SemanticColumnInfo(
+        name="notes",
+        role="text",
+        n_nonnull=3,
+        nunique=3,
+        high_cardinality=False,
+        preserve=False,
+        free_text=True,
+        numeric_like=False,
+        boolean_like=False,
+        money_like=True,
+        unit_like=False,
+        identifier_like=False,
+    )
+    assert expert.applies(info) is False