Skip to content
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.

Sign up for GitHub

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

Jump to bottom

inverse_transform Xt argument consistency #27654

Closed
wd60622 opened this issue Oct 24, 2023 · 6 comments · Fixed by #28756
Closed

inverse_transform Xt argument consistency #27654

wd60622 opened this issue Oct 24, 2023 · 6 comments · Fixed by #28756
Labels
API

Comments

@wd60622
Copy link
Contributor

wd60622 commented Oct 24, 2023

Describe the issue linked to the documentation

Some of the inverse_transform methods take Xt as an argument whereas others take X. Is there are reason for the differences in the names?

Noting the cases here: https://github.com/search?q=repo%3Ascikit-learn%2Fscikit-learn%20%22def%20inverse_transform%22&type=code

Suggest a potential alternative/fix

Stick to Xt in all cases
@wd60622 wd60622 added Documentation Needs Triage Issue requires triage labels Oct 24, 2023
@fparisio
Copy link

fparisio commented Nov 1, 2023

@wd60622 JTBC you recommend refactoring all inverse_transform args X --> Xt?

@wd60622
Copy link
Contributor Author

wd60622 commented Nov 1, 2023 •
edited

@wd60622 JTBC you recommend refactoring all inverse_transform args X --> Xt?

Yes. I read Xt as the result of something.transform(X), so X = something.inverse_transform(Xt). However, a simple renaming could break a lot of code. But it would provide a standard interface for the inverse_transform method.

# Has Xt arg
pipeline = make_pipeline(StandardScaler())
Xt = pipeline.fit_transform(X)
X_again = pipeline.inverse_transform(Xt=Xt)

# inverse_transform takes X instead of Xt
transformer = StandardScaler()
Xt = transformer.fit_transform(X)
X_again = transformer.inverse_transform(Xt=Xt)
# TypeError: inverse_transform() got an unexpected keyword argument 'Xt'

@glemaitre
Copy link
Member

I think that we got a similar refactoring to replace Y by y for consistency (#27666). So I am fine with carrying on this deprecation.

@glemaitre glemaitre added API and removed Needs Triage Issue requires triage Documentation labels Nov 3, 2023
@wd60622
Copy link
Contributor Author

wd60622 commented Nov 3, 2023

I think that we got a similar refactoring to replace Y by y for consistency (#27666). So I am fine with carrying on this deprecation.

Nice, thanks for clarifying. If I can help out in anyway, I'll gladly make a PR

@glemaitre
Copy link
Member

Feel free to make a PR. You can check how this is handled in the other PR to know how to deprecate Xt without throwing too many warnings.

@wd60622 wd60622 mentioned this issue Apr 2, 2024
Merged
@lorentzenchr
Copy link
Member

Conclusion in #28756 (comment) and following comments:
We want the argument if inverse transform to be X and make it happen by the standard deprecation (deprecate in version 1.x, remove deprecation and carry out the final change in 1.x+2).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
API
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

API Standardize X as inverse_transform input parameter wd60622/scikit-learn
4 participants
@glemaitre @lorentzenchr @wd60622 @fparisio