Skip to content

Extend non-fitted error check to cover inverse_transform#953

Open
TemidayoA wants to merge 3 commits into
feature-engine:mainfrom
TemidayoA:test/420-inverse-transform-non-fitted-error
Open

Extend non-fitted error check to cover inverse_transform#953
TemidayoA wants to merge 3 commits into
feature-engine:mainfrom
TemidayoA:test/420-inverse-transform-non-fitted-error

Conversation

@TemidayoA

Copy link
Copy Markdown

What this does

Part of #420 (the inverse_transform half — get_feature_names_out checks were added in #519).

The shared estimator check check_raises_non_fitted_error only verified that transform() raises NotFittedError before fit(). Every transformer that implements inverse_transform already guards it (via _check_transform_input_and_state / check_is_fitted), but no test covered that behaviour, so a regression would go unnoticed.

This extends the shared check to also call inverse_transform on an unfitted clone. Transformers that deliberately don't support inversion (OneHotEncoder, StringSimilarityEncoder) raise NotImplementedError, which the check accepts.

Because the check runs through check_feature_engine_estimator, all transformers across all modules are covered automatically — no per-transformer test changes needed.

Verification

  • Full test suite: 2001 passed
  • Sanity check that the extended check bites: a dummy transformer with an unguarded inverse_transform fails it; real transformers pass
  • flake8 clean on the changed file

The shared estimator check only verified that transform() raises
NotFittedError before fit. Every transformer that implements
inverse_transform already guards it, but nothing tested that, so a
regression would go unnoticed.

The check now also calls inverse_transform on an unfitted clone,
accepting NotImplementedError for transformers that deliberately do
not support inversion.

Related to feature-engine#420
@TemidayoA

Copy link
Copy Markdown
Author

Note on the CI failures on the first run, for whoever reviews:

  • test_feature_engine_py39: all 51 failures are urllib.error.HTTPError: HTTP Error 403: Forbidden in the test_sklearn_wrapper dataset-download tests — a network flake on the runner, unrelated to this PR's diff (the non-fitted checks themselves passed; 1950 tests passed in the same job). Pushed an empty commit to retrigger.
  • test_type: also failing on main's latest pipeline (build 28828), and this PR only touches tests/, which mypy doesn't check — pre-existing.
  • upload_codecov: downstream of the failed job.

# Test when fit is not called prior to inverse_transform.
transformer = clone(estimator)
if hasattr(transformer, "inverse_transform"):
with pytest.raises((NotFittedError, NotImplementedError)):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @TemidayoA

Failing on both NotFittedError and NotImplementedError is a bit dirty, because we won't know if it fails for one or the other, so it won't detect changes that we do looking forward. If there are transformers that have the attribute inverse_transform and it's not implemented, we need to fail specifically for those with the NotImplementedError and for the rest with the NotFittedError

Address review: rather than accepting either NotFittedError or
NotImplementedError, determine per transformer which one is expected and
assert exactly that, so a transformer whose inverse_transform stops
guarding fit is no longer hidden.

A transformer's inverse_transform is treated as not implemented when a
fitted clone raises NotImplementedError; those are expected to raise
NotImplementedError before fit, all others NotFittedError. The
classification fits on a dataframe with categorical and datetime
features so every transformer reaching this check can be fitted.
@TemidayoA

Copy link
Copy Markdown
Author

Thanks @solegalli, that's a fair point — the tuple would have hidden exactly the regressions this check exists to catch. Fixed in the latest commit.

The check now determines, per transformer, which error it should raise and asserts only that one:

  • inverse_transform is treated as not implemented when a fitted clone raises NotImplementedError (OneHotEncoder, RareLabelEncoder, StringSimilarityEncoder) → asserts NotImplementedError.
  • every other transformer → asserts NotFittedError.

I detect implementation status on a fitted clone rather than a hardcoded list, so it stays correct for the conditional cases too (e.g. SklearnTransformerWrapper, which checks fit first and so raises NotFittedError before fit regardless of the wrapped transformer, and MeanEncoder whose default config is implemented).

I confirmed it's now strict: a transformer whose inverse_transform is implemented but drops its fit-guard (raising NotImplementedError before fit) now fails the check, where the tuple passed it. Full suite: 2001 passed, flake8 clean.

The classification fits on test_df(categorical=True, datetime=True) so every transformer that reaches this check can be fitted — the same input the neighbouring get_feature_names_out check already uses.

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.

2 participants