MNT SLEP6 remove args that shouldn't be included in the routing#29920
MNT SLEP6 remove args that shouldn't be included in the routing#29920adam2392 merged 6 commits intoscikit-learn:mainfrom
Conversation
adam2392
left a comment
There was a problem hiding this comment.
A few thoughts:
It seems this requires every class to define something along the lines of:
__metadata_request__score = {"Something": metadata_routing.UNUSED}
Where something can be like check_input, X_test, …. It seems somewhat tedious, and we could easily miss these lines of code in the future. But I also don't see another way around it.
Is there an easy way to test this for future estimators?
Can we just reject certain keywords within metadata? But I guess this would restrict users as well.
|
Yeah we certainly don't want to have an "allowed list" kinda thing. Looking at the estimators out there like LightGBM, they have a wild range of things they pass to For new estimators, or whenever we change something, if we have something which by mistake ends up in the metadata routing machinery, it would create a corresponding Also, looking at the things removed in this PR, other than |
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
|
@adam2392 does this look good to you to merge? |
| # raw_documents should not be in the routing mechanism. It should have been | ||
| # called X in the first place. |
There was a problem hiding this comment.
Doesn't need to be handled here, but I wonder does this warrant us renaming this to X to be consistent across the codebase, or low-priority?
Same for the other regions you mentioned.
|
I guess I will do the green button :) |
Fixes #26505
This cleans up arguments which [probably] shouldn't be included in the routing. Some of them we might want to rename/deprecate (happy to do so in the same PR if that's how we want to proceed).
After this cleanup, the following script gives this output, which I think is okay(?)
cc @OmarManzoor @adam2392 @glemaitre