Skip to content

Merge LogCpTransformer into LogTransformer with C parameter#970

Open
adityaanikam wants to merge 8 commits into
feature-engine:mainfrom
adityaanikam:merge-log-transformers-957
Open

Merge LogCpTransformer into LogTransformer with C parameter#970
adityaanikam wants to merge 8 commits into
feature-engine:mainfrom
adityaanikam:merge-log-transformers-957

Conversation

@adityaanikam

Copy link
Copy Markdown

Closes #957

Summary

Merges LogCpTransformer into LogTransformer as approved in the issue
discussion. LogTransformer gains a C parameter ("auto" / int / float /
dict) mirroring LogCpTransformer's exact semantics, defaulting to C=0 so
existing LogTransformer() usage is unchanged. LogCpTransformer becomes a
thin subclass defaulting to C="auto", noted as being consolidated into
LogTransformer in favor of LogTransformer(C=...).

Backward compatibility

  • LogTransformer()'s behavior and error message/timing are unchanged:
    fit-time validation still fires exactly when C resolves to 0 (the new
    default), with the original message.
  • LogCpTransformer's numerical behavior and C_ computation are unchanged.

Things worth flagging

  1. LogCpTransformer's base= error message lost its dynamic
    Got {base} instead. suffix — it now uses LogTransformer's original
    plain message, since that's the class whose wording was promised
    unchanged. Updated the corresponding test to match.
  2. LogCpTransformer gets its own _more_tags()/__sklearn_tags__()
    overrides resetting to the un-xfailed tags, so it doesn't silently
    inherit LogTransformer's zero-value xfail exemptions that it never
    needed (its "auto" default always finds a valid shift).
  3. There's no existing convention in this codebase for deprecating a class
    (searched — no precedent), so I kept the notice to a plain docstring
    .. note:: rather than inventing a formal .. deprecated:: version
    directive. I also didn't touch docs/user_guide/transformation/ LogCpTransformer.rst, which still describes it as fully independent —
    happy to update that in this PR or a follow-up, whichever you'd prefer.

The 0-handling-without-a-declared-constant question from the issue is left
for a follow-up, per the discussion.

Testing

pytest tests/test_transformation/ — 117 passed, including a new regression
test proving the C=0 default preserves the original fail-fast contract.

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.

[MNT] join LogTransformers into 1 class

1 participant