Skip to content
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

Merged
merged 182 commits into from Nov 25, 2022

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Jul 28, 2020

Add a PredictionErrorDisplay which is useful when dealing with regression.

Edit: Partially addresses #16608.

@NicolasHug
Copy link
Member

i'm getting baited into reviewing this but it doesn't seem finished yet... mark as WIP @glemaitre ? ;)

@glemaitre
Copy link
Member Author

Ups sorry. This is WIP :)

@glemaitre glemaitre changed the title FEA add plot_prediction_error [WIP] FEA add plot_prediction_error Jul 29, 2020
@glemaitre
Copy link
Member Author

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 :)

@glemaitre glemaitre changed the title [WIP] FEA add plot_prediction_error FEA add plot_prediction_error and PredictionErrorDisplay Jul 30, 2020
@glemaitre
Copy link
Member Author

glemaitre commented Jul 30, 2020

@NicolasHug @thomasjpfan I think the PR is ready to get some attention.
Open to any suggestion regarding the API

@glemaitre glemaitre closed this Jul 30, 2020
@glemaitre glemaitre reopened this Jul 30, 2020
Copy link
Member

@NicolasHug NicolasHug left a 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

sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
examples/compose/plot_transformed_target.py Outdated Show resolved Hide resolved
@glemaitre
Copy link
Member Author

It seems that error plots are really just a matter of calling predict and scatter so it's already quite simple.

I agree with these statements. Basically, I wanted my arguments were the following:

  • it is a useful plot that you want to use often in teaching (we need to it during developing our MOOC) or even in practice.
  • the predict and scatter are actually the easy part. What I most dislike with some plots that I saw around: (i) the wrong diagonal is shown and (ii) the aspect ratio of the plot is not squared.

So I agree that this is more of a convenience function.

@jnothman
Copy link
Member

And I like its convenience! I am not sure we need to limit our plotting utilities to things that are difficult to achieve without.

@glemaitre
Copy link
Member Author

I addressed the issue pointed out by @ogrisel

Copy link
Member

@lorentzenchr lorentzenchr left a 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.

doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Nov 24, 2022

I think that using kind="residuals" as the default is indeed a good idea because the class name PredictionErrorDisplay is about "prediction error" which better "residuals vs predicted" than "actual vs predicted".

Copy link
Member

@ogrisel ogrisel left a 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.

sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
examples/model_selection/plot_cv_predict.py Outdated Show resolved Hide resolved
examples/model_selection/plot_cv_predict.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
glemaitre and others added 4 commits November 24, 2022 13:48
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Copy link
Member

@ogrisel ogrisel left a 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.

doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
sklearn/metrics/_plot/regression.py Outdated Show resolved Hide resolved
@jeremiedbb
Copy link
Member

Let's go !

We can still improve some wordings later if needed.

Thanks @glemaitre !

@jeremiedbb jeremiedbb merged commit 40d7d88 into scikit-learn:main Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Guillaume's pet
WAITING FOR REVIEW
Development

Successfully merging this pull request may close these issues.

None yet