api(skore): Improve API related to coefficients feature importance #1995
Description
What would you like to say?
In #1948, we introduce a new display for coefficients of linear model. I did not get time to provide review and I have the following comments that I think should be addressed:
- The display should be documented in the API and user guide. I see that we modify the example which is great.
- The name
FeatureImportanceDisplayis not right: I would expect this class to have akindparameter to be able to select the type of feature importance I want to compute. However, in terms of API, we already do this job when accessing the method, e.g.report.feature_importance.coefficients(). So we are going to have a specialize display for each type of importance and I think it makes sense because the plotting will be different and one class to rule all of them will not be even more difficult to maintain. - I see a reference to the report via the
_parentargument. I think that we should avoid that. The display should be independent as much as possible. The only thing that we need is actually the type of report that could be a string and otherwise, all the other information could be passed as public attribute of the display. - The current attribute
_parentand_coefficientsare private but we can expose them as for other display. - In general, I think that we can store all the information in a single dataframe. There is only the case where we want to compare coefficients of two models that do not have shared features: (i) we should check for it, (ii) we can still plot the results but they should not be on the same plot but in two different plots. I see that we already kind of manage several plots but we should actually have a single figure with several axis. I recall that there is an issue discussion an API to specify the layout and I think that it would be handy to have this API, to properly plot here.
- In terms of rendering, I think that we should make the feature names on the y-axis by default. It allows to have a more compact view, having name that you can read with turning your head or rotating the label.
- We should also despine the axis and we don't really need an axis for the name but only the name aligned with the box or the coefficient)
- We should drop the underlying grid.