-
Notifications
You must be signed in to change notification settings - Fork 97
feat: Add a plot to ComparisonReport summarize display #1816
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
base: main
Are you sure you want to change the base?
Conversation
d1f7858 to
7b1014f
Compare
I have to use this variable in another PR (#1816), and I find it not clean to have to copy/paste the same information in different classes. Refactoring this variable + the methods related to the helpers. More could be done having a look at the metrics and the displays, but no real value for now. Let's wait for it to be needed.
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.
It is a quick review. A couple of additional thoughts:
- we will want to have some logspace sometimes. In scikit-learn, we came with an heuristic with the time for the validation curve: https://github.com/scikit-learn/scikit-learn/blob/d4d4af8c4/sklearn/model_selection/_plot.py#L111-L117 I think that we can retake this implementation
- we need to put some thoughts regarding score vs. loss.
- I think that we need to tackle the following issue first: #1837
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
skore/src/skore/sklearn/_plot/metrics/metrics_summary_display.py
Outdated
Show resolved
Hide resolved
About this, it looks like an improvement to add this option imo. It could be a good first issue. Since we have a workshop soon, what keeping it for this workshop?
If you think that this other issue can be done soon, let's wait indeed! Otherwise, I don't really like keeping PR open for a while, we tend to forget the code and the context.
Do you have the pareto front in mind? |
It is 5 lines of code already tested somewhere else. I would not bother since I already know that we want it.
Not really, it was more on the direction of the loss/score. But I think we can just align the implementation with the display on the hub. In terms of implementation, I don't like a partial implementation. |
Co-authored-by: Guillaume Lemaitre <[email protected]>
Co-authored-by: Guillaume Lemaitre <[email protected]>
12f0b83 to
bf74554
Compare
closes #1421 Created a separated dataframe for features importances and rename the features by string replace operations. Changes were made for two figures: `engineered_ridge_report`
|
@glemaitre, can you be more precise about your expectations about the following sentence please?
So far, the only difference with the hub is the display of the pareto front and the colored aspect on the "worse" side of the front.
|
|
Ready for a new round of review @glemaitre :) ! (still in draft because still waiting for your PR first). |

The plot of this display will be a pair plot to visualize one metric against another.
This is part of the narrative "I have several models, how can I choose the best one?".
Closes #1340