ENH add possibility to have a callable for verbose_feature_names_out of ColumnTransformer#28934
Conversation
|
cc @thomasjpfan pretty sure we had the conversation about this at some point. |
|
Hey, just added the possibility to use string format. I also added unit tests. One of the test I created creates name clashing, and I remember seeing something about that somewhere in sklearn's doc. However, I can't seem to put my hand on it. From the behaviour of ColumnTransformer, I know it is not important since it uses np arrays rather than pandas dataframe, but do you think we should issue a warning somewhere in the doc? -- EDIT -- I think it is supposed to raise an error, but the test passed. |
thomasjpfan
left a comment
thomasjpfan
left a comment
There was a problem hiding this comment.
One of the test I created creates name clashing, and I remember seeing something about that somewhere in sklearn's doc.
Name clashing should raise an error.
I'm not decided on supporting both callable and string. If the string format can support 90% of the use cases, then I'm okay with just having the string formatting.
|
The added complexity here for callable is small. I agree with @thomasjpfan on string formats probably enough, but since the added complexity is small, I don't mind either way. |
|
what are the next steps for this PR? |
thomasjpfan
left a comment
thomasjpfan
left a comment
There was a problem hiding this comment.
Please add an entry to the change log at doc/whats_new/v1.6.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.
|
Thank you! It is all done :) |
There was a problem hiding this comment.
Minor comment, otherwise LGTM
There is still a lint issue: https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=68161&view=logs&j=32e2e1bb-a28f-5b18-6cfc-3f01273f5609&t=fc67071d-c3d4-58b8-d38e-cafc0d3c731a
MarcBresson
force-pushed
the
main
branch
3 times, most recently
from
0d94b69 to
2abd3bc
Compare
| printed as it is completed. | ||
|
|
||
| verbose_feature_names_out : bool, default=True | ||
| verbose_feature_names_out : bool, str or Callable[[str, str], str], default=True |
There was a problem hiding this comment.
The rendered version of this now looks like:
I think it makes sense to make a bullet point list for the 3 cases.
There was a problem hiding this comment.
Ok it should be good. I'm gonna try to find how to compile the doc.
MarcBresson
force-pushed
the
main
branch
5 times, most recently
from
f13055e to
d4294da
Compare
…of ColumnTransformer
…ColumnTransformer also: improve doc for the parameter, fix validation
…erbose_feature_names_out description
…les in back quotes Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
|
@adrinjalali don't hesitate to review. I will rebase my branch right before merging so you can still see that everything passes. |
3 participants
What does this implement?
This brings the possibility to pass a callable to the
verbose_feature_names_outparameter ofColumnTransformer. Instead of the new feature name being "transormer_name__feature_name", we could have "feature_name$this is amazing$TRANSFORMER_NAME".Any other comments?
I have a few questions:
.. versionchanged?In advance, thank you for your time.