Merge LogCpTransformer into LogTransformer with C parameter#970
Open
adityaanikam wants to merge 8 commits into
Open
Merge LogCpTransformer into LogTransformer with C parameter#970adityaanikam wants to merge 8 commits into
adityaanikam wants to merge 8 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #957
Summary
Merges
LogCpTransformerintoLogTransformeras approved in the issuediscussion.
LogTransformergains aCparameter ("auto"/ int / float /dict) mirroring
LogCpTransformer's exact semantics, defaulting toC=0soexisting
LogTransformer()usage is unchanged.LogCpTransformerbecomes athin subclass defaulting to
C="auto", noted as being consolidated intoLogTransformerin favor ofLogTransformer(C=...).Backward compatibility
LogTransformer()'s behavior and error message/timing are unchanged:fit-time validation still fires exactly when
Cresolves to0(the newdefault), with the original message.
LogCpTransformer's numerical behavior andC_computation are unchanged.Things worth flagging
LogCpTransformer'sbase=error message lost its dynamicGot {base} instead.suffix — it now usesLogTransformer's originalplain message, since that's the class whose wording was promised
unchanged. Updated the corresponding test to match.
LogCpTransformergets 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 neverneeded (its
"auto"default always finds a valid shift).(searched — no precedent), so I kept the notice to a plain docstring
.. note::rather than inventing a formal.. deprecated::versiondirective. 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 regressiontest proving the
C=0default preserves the original fail-fast contract.