-
Notifications
You must be signed in to change notification settings - Fork 101
enh: Added subplot functionality for metric displays #1720
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
| report_type=self.report_type, | ||
| ) | ||
|
|
||
| # Handle subplot creation |
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 would make more sense to put the subplots logic in each _plot_<report_type> method
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.
Well, in that case, will look into prediction_error and precision_recall_curve metrics as well
auguste-probabl
left a comment
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; Some refactoring is needed
This comment was marked as outdated.
This comment was marked as outdated.
|
In your second example with the folds it looks like there are only 2 columns, but the code says |
No, because actually in that example, there isn't any mention of the number of columns to begin with. For plotting the fig = display.plot(kind="actual_vs_predicted", subplots=True, figsize=(14, 10))
fig.suptitle("Actual vs Predicted Values Across CV Folds", fontsize=16, y=0.98)Notice how there isn't any mention of the |
Can you update the PR description to make it clear which code leads to which plot? |
Starting from probabl-ai#1669, it is sometimes possible to draw plots onto (a `ndarray` of) several Axes rather than just a single Axes. In fact, starting from probabl-ai#1720, the same data might be drawn on one or the other depending on user parameters. In this context, allowing the user to pass `ax` might add too much complexity compared to how useful it might be; it is considered an advanced use-case which is difficult to motivate. This PR thus removes the ability to pass user-created `ax`.
Starting from probabl-ai#1669, it is sometimes possible to draw plots onto (a `ndarray` of) several Axes rather than just a single Axes. In fact, starting from probabl-ai#1720, the same data might be drawn on one or the other depending on user parameters. In this context, allowing the user to pass `ax` might add too much complexity compared to how useful it might be; it is considered an advanced use-case which is difficult to motivate. This PR thus removes the ability to pass user-created `ax`.
Starting from probabl-ai#1669, it is sometimes possible to draw plots onto (a `ndarray` of) several Axes rather than just a single Axes. In fact, starting from probabl-ai#1720, the same data might be drawn on one or the other depending on user parameters. In this context, allowing the user to pass `ax` might add too much complexity compared to how useful it might be; it is considered an advanced use-case which is difficult to motivate. This PR thus removes the ability to pass user-created `ax`.
Starting from probabl-ai#1669, it is sometimes possible to draw plots onto (a `ndarray` of) several Axes rather than just a single Axes. In fact, starting from probabl-ai#1720, the same data might be drawn on one or the other depending on user parameters. In this context, allowing the user to pass `ax` might add too much complexity compared to how useful it might be; it is considered an advanced use-case which is difficult to motivate. This PR thus removes the ability to pass user-created `ax`.
|
[automated comment] Please update your PR with main, so that the |
|
@waridrox, do you want to continue this PR, or should we take it over and iterate from here? |
Hi @MarieSacksick, I'm willing to continue if the current implementation is fine. I was waiting on some follow ups from other PRs hence I converted this one to draft :) |
|
Seems like a lot has been iterated over the past few months. Is this still something to be worked upon @MarieSacksick? I can update the implementation to resolve merge conflicts or @GaetandeCast do you have any plans to work on this? |
|
While working on #2152, then I think that the scope for the subplot is a bit clearer. @waridrox I think it would be easier to go step by step:
The API would be something like: report = *Report(...)
display = report.metrics.roc()
display.plot() # have a reasonable default
display.plot(subplots_by="label") # one class label per axis
display.plot(subplots_by="estimator_name") # one estimator by axisThe PR that I linked is still work in progress but it should provide a good overview of what we can do even if it is on the feature importance. |
Closes #1445. Converting to draft to iterate upon after #1669, #1701, #1709
Implementation
Added subplot support to the main metric display classes:
PredictionErrorDisplay,RocCurveDisplay, andPrecisionRecallCurveDisplay. Instead of overlaying all results on a single axis, one can visualize metrics across multiple subplots such as - per fold, per estimator, per class.Parameters:
.plot()methods now acceptsubplots,nrows,ncols, andfigsizeparameters.subplots=Truetriggers subplot creation;nrows,ncols, andfigsizeallow custom grid layouts.Automatic Layout Calculation:
utilsforecastapproach) and computes rows via ceiling division.nrowsorncolsisNoneto avoid type errors.Subplotting based on different contexts:
Axis Sharing:
Backward-compatibility: (TBD: whether to keep this or discard it completely)
subplots=False(which is the default option), behavior is unchanged.self.ax_param is always set to the first subplot.Tests:
Usage
References
TODO:
Sample plots following the above gist code ^^
nrowsandncols):nrows=1,ncols=3):nrowsandncols):nrows=1,ncols=3):nrowsandncols):nrows=1,ncols=3):