Conversation
Codecov Report
@@ Coverage Diff @@
## main #1483 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 223 223
Lines 15100 15139 +39
=========================================
+ Hits 15093 15132 +39
Misses 7 7
Continue to review full report at Codecov.
|
jeremyliweishih
left a comment
There was a problem hiding this comment.
Sweet looks good to me. Only extra thing I would mention would that we should use this in the docs when we get to it.
angela97lin
left a comment
There was a problem hiding this comment.
Sweet, the graphs look amazing! Just left a tiny comment about the axis name :d
evalml/model_understanding/graphs.py
Outdated
| # Let plotly pick the best date format. | ||
| layout = _go.Layout(title={'text': "Prediction vs Target over time"}, | ||
| xaxis={'title': 'Time'}, | ||
| yaxis={'title': 'Target Values'}) |
There was a problem hiding this comment.
Really nit-picky: could we name the y-axis something else since we're graphing both target and prediction values?
There was a problem hiding this comment.
Hehe good point. I'll change it to "Target Values and Predictions"!
6c120e8 to
c2d1b05
Compare
|
Good point @jeremyliweishih ! I updated #1384 to mention showing how to use the model understanding module for time series. |
dsherry
left a comment
There was a problem hiding this comment.
@freddyaboulton I left a couple suggestions and some thoughts, but once those are resolved LGTM!
| graph_partial_dependence, | ||
| graph_prediction_vs_actual | ||
| graph_prediction_vs_actual, | ||
| graph_prediction_vs_target_over_time |
There was a problem hiding this comment.
@freddyaboulton do you like "graph_prediction_vs_actual" or "graph_prediction_vs_target" more? We should use the same name for both of these.
I don't feel strongly about it, but I am more used to "prediction vs actual"
There was a problem hiding this comment.
Now that I've seen predicted vs actual, I agree that that sounds much better :o
There was a problem hiding this comment.
Sounds good! I preferred using "target" in the function name since that's what we mention in the docstrings and axis labels but I'll change the name to actual for consistency.
evalml/model_understanding/graphs.py
Outdated
| return _go.Figure(layout=layout, data=data) | ||
|
|
||
|
|
||
| def graph_prediction_vs_target_over_time(pipeline, X, y, dates): |
There was a problem hiding this comment.
I commented about this on the issue, but to reiterate: its fine to leave this as-is and expect dates as a 4th arg, but heads up that @angela97lin is in the process of standardizing all the util/graph methods to use DataTables, in which case X.time_index should get you the dates.
| y (pd.Series): Target values to compare predictions against. | ||
|
|
||
| Returns: | ||
| plotly.Figure showing the prediction vs actual over time. |
There was a problem hiding this comment.
@freddyaboulton could we separate this into a data generation method and a graphing method? That way callers with a different UI can use the data generation method, and people who want plotly figs can use this method.
For this particular plot the data generated is fairly simple, just three vectors: predictions, actual and dates
There was a problem hiding this comment.
Yep I thought about doing that at first but thought it wasn't worth it since it was a one-liner. I'll push this up now!
| class NotTSPipeline: | ||
| problem_type = ProblemTypes.REGRESSION | ||
|
|
||
| error_msg = "graph_prediction_vs_target_over_time only supports time series regression pipelines! Received regression." |
c2d1b05 to
fd2732c
Compare
…n_vs_actual_over_time_data
af55d46 to
6164e95
Compare
Pull Request Description
Fixes #1258
Example with a year's worth of (synthetic) data:

Example with a month's worth of data:

After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.