-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add multiseries support to graph_prediction_vs_actual_over_time #4284
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4284 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 355 355
Lines 39096 39154 +58
=======================================
+ Hits 38976 39034 +58
Misses 120 120
|
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.
One small testing change but LGTM otherwise.
fig.update_yaxes(title_text=y.name) | ||
if single_series is not None: | ||
fig.update_layout( | ||
height=600, |
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.
is there any autosizing we can take advantage of? Also not sure if we need the single_series case: can it just match what we already had before for time series?
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.
Is there any autosizing we can take advantage of?
- For python plotly, it doesn't look like there's any autosizing besides doing what I did in line 490 (though correct me if I'm wrong). If I left the parameters blank for the multiseries graph (i.e. don't define x and y) and let it use the default values the graph looks really squished.
- For single series though I can probably leave the parameters blank since it is only one graph.
Also not sure if we need the single_series case: can it just match what we already had before for time series?
- The issue is that for single_series case I want the data that corresponds to that single series.
- In the before case for time series, y is taken from
data["target"]
whereas for single series I want to take it fromcurr_df["target"]
wherecurr_df = data[data["series_id"] == single_series]
- If I wanted to do that with the before code, I'd probably have to add more
if
statements and such so it made more sense to me to have it included with the multiseries case. - Also I'd like for the single series plot to have a different title than the time series plot.
- In the before case for time series, y is taken from
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.
cool - thanks for the explanation. Works for me!
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.
LGTM
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! Just a docstring comment
Pull Request Description
Closes #4285
A small snippet of what
graph_prediction_vs_actual_over_time
looks like for multiseries: