ENH: Display the number and names of output features#31937
ENH: Display the number and names of output features#31937jeremiedbb merged 207 commits intoscikit-learn:mainfrom
Conversation
|
I wonder if I can have feedback before I add/fix more tests. |
|
I see that we have to handle one specific case: 
We have an internal PassThrough transformer that forward the input feature as-is and this it means that we should mention that the output feature are the same as |
|
I find the block a bit big, it takes as much space as the estimator itself. I was also thinking that having the input features would be nice but then it really starts to take a lot of space around the estimator. So I wondered if the features could be intermediate blocks in the diagram, representing both the output features from the previous estimator and the input features for the next estimator. Something like this This way the diagram alternates estimator blocks and data blocks In addition, in this PR or in a following one, the data block could be unfold to show the feature names if available. |
|
One feedback of @ogrisel IRL is to directly show the feature names using the same pattern than "Parameters". I personally agree with @jeremiedbb feedback: I would like something smaller. Also write now, we have to mention "output features" instead of simply "features" because of the ambiguity input/output when attached to the estimator. So the proposal to make the "feature" being blocks leaving on their own is nice I think because there is not ambiguity anymore. |
|
I'll work on this, thanks for the feedback. Just:
Should I add the feature names on this PR? I remember @glemaitre saying that they should be added on a separate PR. |
I want to dissociate it at first but since we are going to create a new block, it might be better to have directly the feature names as well. |
a2c76ba to
9c93171
Compare
I changed that. |
AnneBeyer
left a comment
There was a problem hiding this comment.
Thank you very much for your work, @DeaMariaLeon! This is really not straight forward and the output looks really good!
Sorry for taking so long, I checked all included examples visually and tried to understand the code changes. Here is my review so far, only some small suggestions. I still have two files to go, but I didn't want to keep you waiting even longer.
|
|
||
| if est_block.names == "NoneType(...)": | ||
| est_block.names = "passthrough" | ||
|
|
There was a problem hiding this comment.
+1 for adding a FIXME/TODO comment for a follow-up PR.
In lines 348 and 356
not est_block.names = "passthrough"is part of the condition being checked, but it seems that it never actually is "passthrough" (at least when debugging the code snippet from above), so either this needs to be set already in _get_visual_block (or even in ColumnTransformer's_sk_visual_block_()?) or this condition in the lines above is actually redundant? I haven't found the exact location yet where this NoneType is set. It seems to be related to names=estimator.__class__.__name__, in _get_visual_block when estimator is (None,), but I got lost in the recursion. Maybe you can find it faster?
AnneBeyer
left a comment
There was a problem hiding this comment.
And here is my feedback on the remaining two files.
ogrisel
left a comment
There was a problem hiding this comment.
A small unfinished review I had pending on my side and forgot to submit.
Thanks, I removed that. I believe a FIXME is not needed (unless the tests fail of course). |
AnneBeyer
left a comment
There was a problem hiding this comment.
LGTM now, thank you @DeaMariaLeon!
Could we merge this @ogrisel?
|
@DeaMariaLeon I checked the modified examples and when the html repr is too large, there's no horizontal scrollbar: I guess we didn't notice so far because we only showed small displays. It doesn't seem to be a bug introduced in this PR but it makes the rendering of the modified examples not great. I don't know if and how it would be possible to have a scrollbar here. |
|
or maybe for now we don't show the estimator in this example. |
|
@jeremiedbb are we sure that there should be an horizontal scroll bar? |
|
What's your browser ? I'm on firefox and I can't |
|
chrome and safari, both work. It's not introduced with this PR in any case because if you open this example in main, there is no horizontal scroll bar either: https://scikit-learn.org/stable/auto_examples/compose/plot_column_transformer_mixed_types |
|
Indeed. And on firefox I can't move it either. I'm goind to open a dedicated issue. Let's merge this one ! |
|
oh.. but I'm doing it with the trackpad, not the mouse! it works that way with firefox too (not the mouse but the trackpad inside the laptop). Thanks a lot everybody! |
hum, but it's not a laptop for me, there's no trackpad 😄 |
Reference Issues/PRs
Towards #26595
Any other comments?
Example
