-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove TargetTransformer class #2833
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2833 +/- ##
=====================================
Coverage 99.8% 99.8%
=====================================
Files 297 297
Lines 27741 27741
=====================================
Hits 27664 27664
Misses 77 77
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love these cleanup PRs, keep them coming 😂 ! LGTM, thank you! 👍
@@ -1631,13 +1628,13 @@ def test_component_modifies_feature_or_target(): | |||
for component_class in all_components(): | |||
if ( | |||
issubclass(component_class, BaseSampler) | |||
or issubclass(component_class, TargetTransformer) | |||
or hasattr(component_class, "inverse_transform") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question: Do you think this will hold true long term? It's true for our components now but... 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I do think that if we have an inverse_transform
method, then it'll modify the target rather than the features. It doesn't seem to make sense to have a transformer have an inverse_tranform
method for the features. I also don't think it'd make sense to introduce an inverse_transform
method if we don't alter the target at all. Logically, it seems like this should still hold long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add inverse_transform
as a method to Transformer
? The "default" behavior is a no-op and if we want to enforce that transformers that modify the target override that behavior we can do something like
def inverse_transform(self, y):
if self.modifies_target:
raise NotImplementedError("Transformers that modify the target must override inverse_transform")
return y
That way this behavior always holds. Or we don't make this change? Maybe I was being crazy when I filed the issue lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Freddy's suggestion is a good one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
fix #2687