FEA add PredictionErrorDisplay#18020
Conversation
|
i'm getting baited into reviewing this but it doesn't seem finished yet... mark as WIP @glemaitre ? ;) |
|
Ups sorry. This is WIP :) |
|
You can have a look at the API thought. I change examples and I am writing proper test now that I did my example-based development :) |
|
@NicolasHug @thomasjpfan I think the PR is ready to get some attention. |
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @glemaitre,
I only took a very brief look but my impression is that this might be a bit too high-level for scikit-learn.
My understanding of our goal with the plotting utilities is that we allow users to i) compute complicated stuff like PDPs and ii) update the plot styles without having to re-compute the results every time.
It seems that error plots are really just a matter of calling predict and scatter so it's already quite simple.
Just my personal thoughts, no strong opinion anyway
I agree with these statements. Basically, I wanted my arguments were the following:
So I agree that this is more of a convenience function. |
|
And I like its convenience! I am not sure we need to limit our plotting utilities to things that are difficult to achieve without. |
examples/inspection/plot_linear_model_coefficient_interpretation.py
Outdated
Show resolved
Hide resolved
|
I addressed the issue pointed out by @ogrisel |
|
I think that using |
ogrisel
left a comment
There was a problem hiding this comment.
More feedback.
Once this and @lorentzenchr's review comments are addressed, LGTM.
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
ogrisel
left a comment
There was a problem hiding this comment.
Final pass. Other than that and the previous suggestion, LGTM.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
Let's go ! We can still improve some wordings later if needed. Thanks @glemaitre ! |
Add a
PredictionErrorDisplaywhich is useful when dealing with regression.Edit: Partially addresses #16608.