refactor: extract validation block and remove dead complex-data guard in transformers

[shubhrai23]

This PR addresses #155.

While looking into the repeated validation blocks in _transformers.py, I realized why the complex-data guard seemed unreachable. validate_data() returns an object dtype array when it falls back to mixed types (e.g. containing a mix of dates, strings, and complex numbers). Because of this, np.iscomplexobj() evaluates to False (since the overall array dtype is object, not complex), meaning the guard was silently bypassed and acting as pure dead code.

I've refactored the file by:

1. Extracting the duplicated validate_data try-except block into a single _validate_X helper function to DRY things up.
2. Removing the dead iscomplexobj check entirely. Complex numbers now bypass it naturally and are handled downstream by _coerce_currency_to_float (which coerces them to NaN and emits a warning, exactly as expected by the existing test suite).

All tests pass locally.

Closes #155

[Copilot]

🟡 Changes recommended

Repository policy requires an ## [Unreleased] CHANGELOG.md entry for PRs touching philanthropy/, and the updated complex-data behavior wording in transformer docstrings should be corrected to match runtime behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors philanthropy.preprocessing._transformers by extracting the repeated validate_data() try/except (with object-dtype fallback) into a shared _validate_X helper, and removes the redundant np.iscomplexobj complex-data guard that was effectively dead under the current validation behavior.

Changes:

• Added _validate_X() to centralize the validate_data(..., dtype=None) + object-cast retry logic.
• Replaced duplicated inline validation blocks in CRMCleaner and FiscalYearTransformer with calls to _validate_X().
• Removed the trailing np.iscomplexobj(...) checks and their ValueError("Complex data not supported") raises.

File summaries

File	Description
philanthropy/preprocessing/_transformers.py	DRYs up validation logic via _validate_X and removes the redundant complex-object guard in both transformers.

Review details

Suppressed comments (3)

philanthropy/preprocessing/_transformers.py:198

• This docstring claims complex data always raises, but complex values can survive validation when they arrive inside object-dtype columns (and are handled downstream). Update the wording to reflect that the ValueError is for inputs that fail validation (e.g., complex-dtype arrays).

            If ``X`` contains complex data.

philanthropy/preprocessing/_transformers.py:306

• This docstring says transform raises whenever X contains complex data, but in practice the ValueError comes from sklearn validation for complex-dtype inputs; complex values inside object-dtype columns can flow through and get coerced. Updating the wording avoids overstating the contract.

            If ``X`` contains complex data.

philanthropy/preprocessing/_transformers.py:277

• The docstring currently states that any complex data in X raises, but the validation logic only reliably raises for complex-dtype inputs; complex values inside object-typed columns may pass and be coerced later. Consider describing the error condition as 'fails validation' to match runtime behavior.

            If ``fiscal_year_start`` is invalid or ``X`` contains complex data.

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[shivamlalakiya]

Read the diff rather than merging on green, since this one touches validation shared by two transformers. It holds up.

The reachability finding is right, and it is right for two independent reasons rather than one. A genuine complex dtype never reaches np.iscomplexobj because scikit-learn's check_array calls _ensure_no_complex_data first and raises with exactly the phrase the except clause re-raises on. And an object-dtype array holding Python complex cells has dtype.kind == "O", so np.iscomplexobj returns False on it too. Both paths miss the guard, which is why nothing in the suite ever executed those four lines.

Checked that the deletion is not removing behaviour the suite was relying on: all four entry points still have a complex-rejection test (test_complex_column_is_re_raised_not_retried covers both fit methods, test_crm_transform_re_raises_complex_column and test_fiscal_year_transform_re_raises_complex_column cover both transform methods), and they pass on scikit-learn's own message. check_complex_data in test_sklearn_compliance.py is unaffected for the same reason. grep -c iscomplexobj is now 0, which clears the done-when condition in #155.

Two small things, neither worth another round trip:

• The changelog bullet went under ### Fixed. It is a refactor with no behaviour change, so ### Changed is its home. I am moving it while dating the changelog for 0.7.1 today rather than asking you to push again.
• refactor: _transformers.py repeats one validation block four times, and its complex-data guard looks unreachable #155 asked for the reachability finding to be posted on the issue before the PR was opened, and CONTRIBUTING.md now asks for a claim comment first (that rule landed the day before this PR). You did report the finding, just in the PR body instead. No harm here, and next time a comment on the issue first keeps two people from doing the same work, which is exactly what happened with fix: remove dead hasattr branch in WealthPercentileTransformer.fit (#168) #175 and fix: remove dead branch in WealthPercentileTransformer.fit (#168) #176 last week.

Thanks. Four copies became one and the dead line that made the coverage report lie is gone.