Plot Step Ahead Projection function #2

LucieContamin · 2023-08-01T21:06:25Z

Create a plot_step_ahead_forecasts() function (create plot_step_ahead_forecasts() #1)
Create a vignette illustrating the function

Still to do:

The function output Plotly type object, we should add a "interactive" parameter that when set to TRUE returns Plotly object, but returns ggplot2 type object when set to FALSE.
Add Testing

- from the example-complex-scenario-hub. "example_worfklow.Rmd"

- add `use_median_as_point` parameter - force forecast_data as "model_output_df" - fix input of plotly function - update plot if no "point" value

to show or not the output plot

- output for 1 model_id in ens_name (truth data and fix error by default) - return plot_model object

- allow none, one or multiple intervals - adapt transparency

- fix opacity - reorganize function (create internal functions) - add top_layer parameter

- hover text format for interactive plot - fix visible binding for global variable

LucieContamin · 2023-10-04T18:10:37Z

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.

nickreich

I am basically fine with the content of this PR as is. My final requests are:

some investigation into why the plotly figure heights are so tall and ideally fixing this so by default they are lower
adding at least some unit tests to the code

I do think we should enforce the unit tests piece prior to merging in the current PR, even though the functionality appears to be there. I don't have the time to do this now, but I would be happy at a future time to help brainstorm a list of possible tests that could be put in place here. E.g. one that came to me on this latest re-review is making sure we throw an error if the column specified by x_col_name argument is not a date.

saraloo · 2023-10-05T16:38:24Z

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?

annakrystalli

Made a number of comments but most are just suggestions. Overall I found nothing critical to resolve but think following some suggestions might make the code easier to follow and reason about.

Also might be a good idea to run styler::tidyverse_style() on any R/*.R and testthat/*.R script to make them more readable.

tests/testthat/test-plot_step_ahead_model_output.R

annakrystalli · 2023-10-27T13:39:17Z

vignettes/plot_projection.Rmd

+- `example_round1.csv`: example of model output for a round associated with the 
+origin date: "2021-03-07" (called "round 1"), target: "incident case", for the
+US national level, from the
+[example-complex-scenario-hub](https://github.com/Infectious-Disease-Modeling-Hubs/example-complex-scenario-hub). The data set also contains an ensemble calculated by applying the 


I think we answered this on slack. While the idea is generally good, in some packages we don't need a full example hub so to keep package sizes as small as possible, I think it is acceptable to only include small and specific test materials to the functionality being tested.

R/plot_step_ahead_model_output.R

annakrystalli · 2023-10-27T15:02:26Z

R/plot_step_ahead_model_output.R

+#' @param all_ens a list with two data frame: one for plain lines,
+#'  one for ribbons plotting (in a wide format) for a unique `model_id` value
+#'  associated with specific color (`ens_color`). NULL is no specific layout
+#'  required


I'm struggling a little to understand the difference between all_plot and all_ens

It's mostly the same format. all_ens is only non NULL when the parameter to set the ensemble to a specific color and contains the data associated with the model_id designated as "ensemble". In this case, the model_id will be excluded from the all_plot object. I split them because they have slightly different plotting parameters and it was easy to set it like this.
However, I am happy to re-think in a future version and make it easier to understand (same as the idea to "modularize" the lapply/if part of the code)

R/plot_step_ahead_model_output.R

annakrystalli · 2023-10-27T15:11:06Z

R/plot_step_ahead_model_output.R

+    subplot <- lapply(facet_value, function(x) {
+      # Data preparation
+      df_point <- all_plot$median[which(all_plot$median[[facet]] == x), ]
+      df_ribbon <- all_plot[names(all_plot) %in% intervals]
+      df_ribbon <- setNames(lapply(df_ribbon, function(df_rib) {
+        df_rib[which(df_rib[[facet]] == x), ]
+      }), names(df_ribbon))
+      if (!is.null(all_ens)) {
+        df_point_ens <- all_ens$median[which(all_ens$median[[facet]] == x), ]
+        df_ribbon_ens <- all_ens[names(all_ens) %in% intervals]
+        df_ribbon_ens <- setNames(lapply(df_ribbon_ens, function(df_rib) {
+          df_rib[which(df_rib[[facet]] == x), ]
+        }), names(df_ribbon_ens))
+      }
+      if (plot_truth & facet %in% colnames(truth_data)) {
+        truth_data <- truth_data[which(truth_data[[facet]] == x), ]
+      }
+
+      # Plot
+      if (x == facet_value[1]) {
+        plot_model <- simple_model_plot(
+          plot_model, df_point, df_ribbon, plot_truth, truth_data,
+          opacity = fill_transparency, top_layer = top_layer,
+          interactive = TRUE, fill_by = fill_by, x_col_name = x_col_name,
+          x_truth_col_name = x_truth_col_name)
+      } else if (facet == fill_by) {
+         plot_model <- simple_model_plot(
+           plot_model, df_point, df_ribbon, plot_truth, truth_data,
+           opacity = fill_transparency, top_layer = top_layer,
+           show_truth_legend = FALSE, interactive = TRUE,
+           fill_by = fill_by, x_col_name = x_col_name,
+           x_truth_col_name = x_truth_col_name)
+      } else {
+        plot_model <- simple_model_plot(
+          plot_model, df_point, df_ribbon, plot_truth, truth_data,
+          opacity = fill_transparency, showlegend = FALSE,
+          top_layer = top_layer, show_truth_legend = FALSE,
+          interactive = TRUE, fill_by = fill_by, x_col_name = x_col_name,
+          x_truth_col_name = x_truth_col_name)
+      }
+      # Ensemble color
+      if (!is.null(all_ens)) {
+        if (x == facet_value[1]) {
+          plot_model <- simple_model_plot(
+            plot_model, df_point_ens, df_ribbon_ens, FALSE, truth_data,
+            line_color = ens_color, opacity = fill_transparency,
+            top_layer = top_layer, interactive = TRUE, fill_by = fill_by,
+            x_col_name = x_col_name, x_truth_col_name = x_truth_col_name)
+        } else if (facet == fill_by) {
+          if (facet == "model_id" & ens_name == x) {
+            plot_model <- simple_model_plot(
+              plot_model, df_point_ens, df_ribbon_ens, TRUE, truth_data,
+              line_color = ens_color, opacity = fill_transparency,
+              top_layer = top_layer, show_truth_legend = FALSE,
+              interactive = TRUE, fill_by = fill_by, x_col_name = x_col_name,
+              x_truth_col_name = x_truth_col_name)
+          }
+        } else {
+          plot_model <- simple_model_plot(
+            plot_model, df_point_ens, df_ribbon_ens, FALSE, truth_data,
+            line_color = ens_color, opacity = fill_transparency,
+            showlegend = FALSE, top_layer = top_layer,
+            show_truth_legend = FALSE, interactive = TRUE, fill_by = fill_by,
+            x_col_name = x_col_name, x_truth_col_name = x_truth_col_name)
+        }
+      }
+      if (!is.null(facet_title)) {
+        if (grepl("top", facet_title)) {
+          y_title <- 1
+          y_anchor <- "top"
+        } else if  (grepl("bottom", facet_title)) {
+          y_title <- 0
+          y_anchor <- "bottom"
+        }
+        if (grepl("left", facet_title)) {
+          x_title <- 0
+          x_anchor <- "left"
+        } else if  (grepl("right", facet_title)) {
+          x_title <- 1
+          x_anchor <- "right"
+        }
+        plot_model <- plotly::layout(
+          plot_model,
+          annotations = list(x = x_title, y = y_title, xref = "paper",
+                             yref = "paper", xanchor = x_anchor,
+                             yanchor = y_anchor, showarrow = FALSE, text = x))
+      }
+      return(plot_model)
+    })


This whole section is quite nested and hard to follow. I wonder if some of it can be modularised and pulled out into smaller more concise functions?

Also although the unnamed function called in lapply only has x as an argument, there are a lot of objects used from the function environment which also make it a bit confusing and hard to follow.

I agree, it will take me some time to test and implement. I am happy to think about it for a future version.

tests/testthat/test-plot_step_ahead_model_output.R

Co-authored-by: Anna Krystalli <annakrystalli@googlemail.com>

LucieContamin added 17 commits July 11, 2023 16:22

add draft plot_step_ahead_forecasts()

5202ebd

- from the example-complex-scenario-hub. "example_worfklow.Rmd"

update plot_step_ahead_forecasts()

1c8234d

- add `use_median_as_point` parameter - force forecast_data as "model_output_df" - fix input of plotly function - update plot if no "point" value

add a plot parameter

9e0457c

to show or not the output plot

Fix hover_text for truth data

62d7cf5

Merge branch 'main' into add_basic_fct

7da11ac

organize tests + update median options

0c883df

fix tests and add valid types

080097f

add plot_truth parameter

23e744b

fix test median

333ae23

add facet parameter(s)

cf05d42

minor fixes

f5ef6e8

- output for 1 model_id in ens_name (truth data and fix error by default) - return plot_model object

add show_legend parameter

8f17d1f

add fill_ and palette parameters

19e1ccf

update "intervals" value

05481e0

- allow none, one or multiple intervals - adapt transparency

update plot function

c652d2a

- fix opacity - reorganize function (create internal functions) - add top_layer parameter

fix legend behavior

82a5ab7

add vignette

6d42504

LucieContamin added this to the Minimal hubverse production release milestone Aug 1, 2023

LucieContamin and others added 12 commits August 9, 2023 11:19

add interactive (only for non-facet plot)

405e9b7

update vignette

827f068

add interactive option for facet

ec74a8d

update vignette

3dcfe69

typo

ec217ed

Merge branch 'main' into add_basic_fct

0f0d1f3

minor edits by Harry

922abbe

minor edits to vignette

e03c265

clarified text

c626f4d

minor fixes

74f4abb

- hover text format for interactive plot - fix visible binding for global variable

Merge branch 'main' into add_basic_fct

86dca43

update README

5d01217

LucieContamin requested review from nickreich and harryhoch October 4, 2023 18:48

adding just a bit more explanatory text for the new x_col_name argument.

d3d3f62

nickreich requested changes Oct 4, 2023

View reviewed changes

rm manual height and update vignette fig.height setting

012af8f

LucieContamin mentioned this pull request Oct 5, 2023

Legend for model_id facet plot #11

Closed

LucieContamin added 10 commits October 5, 2023 14:19

fix plotly behavior if fill_by = facet parameters

d3e39ea

add one_color parameter

1b18deb

add tests on input parameter (+ small fix)

352b7f6

update plot parameter to show_plot

2300db0

fix input facet check

3569355

add tests on output

817d0d0

fix trace order

0cfaff4

update tests

57c7219

update read.csv() for back compatibility

2db0ac8

update read.csv() for back compatibility

07aa2c3

LucieContamin linked an issue Oct 10, 2023 that may be closed by this pull request

Unit testing #8

Closed

fix facet_ncol parameter (update vignette)

7d30cc8

LucieContamin linked an issue Oct 11, 2023 that may be closed by this pull request

create plot_step_ahead_forecasts() #1

Closed

annakrystalli reviewed Oct 27, 2023

View reviewed changes

LucieContamin and others added 4 commits October 27, 2023 14:07

update documentation and style

bbdb005

re-organize functions + fix dependences

9646cc4

Update R/plot_step_ahead_model_output.R

61cec9d

Co-authored-by: Anna Krystalli <annakrystalli@googlemail.com>

style

9878a59

LucieContamin mentioned this pull request Oct 31, 2023

Update tests #13

Open

add small example

0fa1816

LucieContamin merged commit febce6f into main Nov 14, 2023
9 checks passed

LucieContamin deleted the add_basic_fct branch November 14, 2023 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plot Step Ahead Projection function #2

Plot Step Ahead Projection function #2

LucieContamin commented Aug 1, 2023 •

edited

Loading

LucieContamin commented Oct 4, 2023

nickreich left a comment •

edited by LucieContamin

Loading

saraloo commented Oct 5, 2023

annakrystalli left a comment

annakrystalli Oct 27, 2023

annakrystalli Oct 27, 2023

LucieContamin Oct 31, 2023

annakrystalli Oct 27, 2023

LucieContamin Oct 31, 2023

Plot Step Ahead Projection function #2

Plot Step Ahead Projection function #2

Conversation

LucieContamin commented Aug 1, 2023 • edited Loading

LucieContamin commented Oct 4, 2023

nickreich left a comment • edited by LucieContamin Loading

Choose a reason for hiding this comment

saraloo commented Oct 5, 2023

annakrystalli left a comment

Choose a reason for hiding this comment

annakrystalli Oct 27, 2023

Choose a reason for hiding this comment

annakrystalli Oct 27, 2023

Choose a reason for hiding this comment

LucieContamin Oct 31, 2023

Choose a reason for hiding this comment

annakrystalli Oct 27, 2023

Choose a reason for hiding this comment

LucieContamin Oct 31, 2023

Choose a reason for hiding this comment

LucieContamin commented Aug 1, 2023 •

edited

Loading

nickreich left a comment •

edited by LucieContamin

Loading