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.

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

MNT mark non-standard inverse_transform X args as non-metadata (SLEP6) #26503

Merged
merged 8 commits into from
Jun 14, 2023

Conversation

adrinjalali
Copy link
Member

set_{method}_request is generated for methods whenever they have arguments which are not X, y, or Y. But for inverse_transform we have a few places where we call the argument something else. This PR sets them explicitly as UNUSED so that they're ignored by the metadata method generation mechanism and set_inverse_transform_request method is not generated for them.

I don't have a strong preference on adding Xt and maybe yt / Yt to the hard coded list of ignored argument names.

@jeremiedbb
Copy link
Member

I wouldn't mind having a consistent API for inverse_transform, deprecating W in NMF and Xred in AgglomerationTransform in favor of Xt. With a consistent API it would make even more sense to add Xt, yt to the hard coded list of ignored args.

@adrinjalali
Copy link
Member Author

@jeremiedbb done

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

The 2 deprecations will need |API| entries in the changelog. Otherwise I made a few suggestions but looks good overall

sklearn/cluster/_feature_agglomeration.py Outdated Show resolved Hide resolved
sklearn/decomposition/tests/test_nmf.py Outdated Show resolved Hide resolved
sklearn/decomposition/tests/test_nmf.py Show resolved Hide resolved
sklearn/cluster/_feature_agglomeration.py Outdated Show resolved Hide resolved
sklearn/decomposition/_nmf.py Outdated Show resolved Hide resolved
Comment on lines +1274 to +1275
if Xt is None and W is None:
raise TypeError("Missing required positional argument: Xt")
Copy link
Member

Choose a reason for hiding this comment

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

feels a bit weird but I think it's fine since it's only during the deprecation cycle.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I'm doing a TypeError here since python would do the same if user doesn't pass a positional arg.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM.

@glemaitre glemaitre merged commit 83cb686 into scikit-learn:main Jun 14, 2023
26 checks passed
@adrinjalali adrinjalali deleted the slep6/feature-agglomeration branch June 14, 2023 12:59
@adrinjalali adrinjalali mentioned this pull request Jun 14, 2023
12 tasks
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
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.

None yet

3 participants