Closed LucieContamin closed 10 months ago
I think this generally looks good, but I'm concerned that the color brewer plot is unreadable. Can we remove the plot and just say that colors from colorbrewer can be used?
Thank you, I can either:
All the palette from RColorBrewer can be used. The possible palette names are accessible by calling:
and give the command to have all the possible name Thanks. Either change would be fine.
I committed a handful of minor changes to the vignette and one argument name change to the plot_step_ahead_forecasts() function. here are the three items that I see as perhaps things that should be fixed before committing this PR:
* [x] consider a different name for the function, in the spirit of generalizing beyond forecasts. Like `plot_step_ahead_model_output()`? * [x] per comment on the PR, consider making the "fill_by" argument less restricted to model only. * [x] add `x_col_name` argument with a default of "target_date". * [ ] possibly consider using more centrally located example files, although I don't think we should let this be a blocker to merging this.
Thank you for the feedback, I implemented the required changes except for the example files. I am not sure what would be the best solution here.
Found this generally very clear and easy to follow.
I find the attached plot (faceted by model) a bit confusing, with the legend only showing the one model (the first panel). In the interactive plotly, if you then click to not display the model, only the first panel projection disappears (pictured). I would propose to have the default of this plot to not have a legend, but not sure exactly how to set this up practically (tricky with the option default being true for other plots). Another option might be to allow each of the panels to still be a different colour so the legend corresponds to something meaningful?
plot_step_ahead_forecasts()
function (#1)Still to do:
TRUE
returns Plotly object, but returns ggplot2 type object when set toFALSE
.