ENH: Display the number of output features #31937
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
glemaitre
left a comment
There was a problem hiding this comment.
It really looks good. Now I just think that we need to work more in depth for the tests but the code is clean.
| max-height: 10em; | ||
| overflow: auto; | ||
| scrollbar-width: thin; | ||
| padding: .25em .5em; |
There was a problem hiding this comment.
To be inline with other tables. FYI, we don't need to take care about the vertical scrolling bar. The text will not overlap with it.
| padding: .25em .5em; | |
| padding: .25em 0.1rem; |
There was a problem hiding this comment.
@glemaitre this change makes on a jupyter notebook to have a higher space for the other tables. It would look like this:
There was a problem hiding this comment.
FYI, I included the change anyway.
|
|
||
| def test_features_number_not_included(): | ||
| est = LogisticRegression(C=10.0, fit_intercept=False) | ||
| assert "Output features" not in estimator_html_repr(est) |
There was a problem hiding this comment.
Let's create a test_features.py file to add those new test. I think that you need to update those tests. I think that we can test:
- Make sure that we don't show anything when the estimator is not fitted
- Make sure that we have have the boxes when the estimator is fitted
- Make sure that we have the right feature names in the table
- Make sure that it have the expected number of "X features" when having a column transformer in a pipeline
- Make sure that thing works even with
Transformerthat do not provide the full scikit-learn API. In short, we should add asklearn.utils._testing.MinimalTransformerin aPipelineand check that it does not crash.
There was a problem hiding this comment.
Done - but I'm using estimator_html_repr to test. I wonder what think about that.
| # | ||
| print( | ||
| "accuracy of the best model from randomized search: " | ||
| "accuracy of the best model from randomized search: " |
There was a problem hiding this comment.
We should remember to revert this change for the final merge ;)
|
|
||
| estimator_html_repr(model) | ||
|
|
||
|
|
There was a problem hiding this comment.
Copilot wrote the tests that follow.
WDYT?
|
It will be worth adding a changelog for this improvement as well. |
Reference Issues/PRs
Towards #26595
What does this implement/fix? Explain your changes.
This is part of "Display the (public) fitted attributes".
Any other comments?
Example
