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 (SLEP6) remove other_params from provess_routing #26909

Merged
merged 4 commits into from Jul 27, 2023

Conversation

adrinjalali
Copy link
Member

This PR removes the other_params from process_routing, since they can be passed as **kwarg anyway. There used to be some difference between args passed explicitly and the ones passed as kwargs to methods such as fit, but later with the introduction of a more rich MetadataRouter the need for that distinction was removed.

This also renames obj to _obj and method to _method as arguments and makes them positional only. This also reflects quite a bit from Python's API. The rename helps with avoiding collisions between metadata and obj and method as metadata names.

cc @thomasjpfan @OmarManzoor

@github-actions
Copy link

github-actions bot commented Jul 26, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 734548c. Link to the linter CI: here

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

During my initial SLEP006 review, I have found other_params a bit weird. I am happy with this change.

Does all the calls in process_routing in examples/miscellaneous/plot_metadata_routing.py need to change too?

@@ -1412,7 +1412,7 @@ def get_metadata_routing(self):
# given metadata. This is to minimize the boilerplate required in routers.


def process_routing(obj, method, other_params, **kwargs):
def process_routing(_obj, _method, /, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is not the norm, may you add a comment here that explains the purpose of _obj and _method based on #26909 (comment) ?

@adrinjalali
Copy link
Member Author

Nice hint with the example usage, fixed.

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @adrinjalali

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan enabled auto-merge (squash) July 27, 2023 14:23
@thomasjpfan thomasjpfan merged commit 36c5073 into scikit-learn:main Jul 27, 2023
26 checks passed
@adrinjalali adrinjalali deleted the slep6/other_params branch July 27, 2023 17:47
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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