Skip to content

docs: document fit and transform on preprocessing transformers - #40

Merged
shivamlalakiya merged 2 commits into
PhilanthroPy-Project:mainfrom
slegarraga:docs/transformer-docstrings
Aug 7, 2026
Merged

docs: document fit and transform on preprocessing transformers#40
shivamlalakiya merged 2 commits into
PhilanthroPy-Project:mainfrom
slegarraga:docs/transformer-docstrings

Conversation

@slegarraga

Copy link
Copy Markdown
Contributor

Closes #33.

Adds NumPy-style docstrings to fit and transform on:

  • CRMCleaner
  • FiscalYearTransformer
  • WealthPercentileTransformer

The docstrings describe frozen state, return types, and raised errors. WealthPercentileTransformer documents that percentiles are relative to the frozen training distribution in percentile_lookup_.

Verified:

  • missing-docstring check prints MISSING: none then OK
  • pytest --doctest-modules philanthropy passes (27)
  • pytest tests/test_preprocessing.py tests/test_leakage.py passes (49)

@shivamlalakiya

Copy link
Copy Markdown
Contributor

Thanks for this — the parameter/return/raises coverage on these four methods is exactly the shape the rest of the codebase uses, and calling out that percentile_lookup_ freezes the training distribution is the right thing to make explicit.

One correction before this goes in. Both CRMCleaner.transform and FiscalYearTransformer.transform document the return as:

The return type mirrors the input type.

That isn't what the code does. The branch at philanthropy/preprocessing/_transformers.py:128 (and :215) is:

if _get_pandas_output(self):
    return X_df
return X_df.to_numpy()

_get_pandas_output reads the scikit-learn set_output config, not the type of X. With the default config, passing a DataFrame in still gets an ndarray back — so the docstring would send readers the wrong way on the one thing they'd look this up for.

Suggested wording:

X_out : np.ndarray or pd.DataFrame
Cleaned feature matrix. Returns a DataFrame when the transformer is
configured with set_output(transform="pandas"), otherwise an ndarray.

Fix those two spots and I'll merge. Everything else in the PR is accurate as written.

@slegarraga

Copy link
Copy Markdown
Contributor Author

Fixed in b9f9b3d. Both CRMCleaner.transform and FiscalYearTransformer.transform now document that the return is a DataFrame only when set_output(transform="pandas") is configured, otherwise an ndarray. Tests pass (49). Thank you for the precise review!

@slegarraga
slegarraga force-pushed the docs/transformer-docstrings branch from b9f9b3d to 939c739 Compare August 6, 2026 22:50
@shivamlalakiya
shivamlalakiya merged commit bc1c1e6 into PhilanthroPy-Project:main Aug 7, 2026
14 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

Development

Successfully merging this pull request may close these issues.

docs: CRMCleaner, FiscalYearTransformer and WealthPercentileTransformer document neither fit nor transform

2 participants