-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FEA add PredictionErrorDisplay #18020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A partial review. Short on time atm.
I think that using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
PredictionErrorDisplay
which is useful when dealing with regression.Edit: Partially addresses #16608.