mcguinlu / robvis

A package to quickly visualise risk-of-bias assessment results
https://mcguinlu.github.io/robvis/
Other
57 stars 21 forks source link

Error in check_cols() with 8-column data frame when calling rob_traffic_light() #118

Closed earcanal closed 1 year ago

earcanal commented 3 years ago

rob_traffic_light() has been working with my 8-column (Study, D1-D5, Overall, Weight) data frame for quite a while. I upgraded robvis from this repo today and I now get the following error:

    Error in check_cols(data = data, max_domain_column = max_domain_column, : The number of columns in your data (8) does not match the number expected for this tool when using overall = TRUE. The expected number of columns is 7: a "Study" column, 6 "Domain" columns, and an "Overall" column.
5.
stop("The number of columns in your data (", ncol(data), ") does not match the number expected for this", " tool when using overall = ", overall, weighted_text, ". The expected number of columns is ", domain_text)
4.
check_cols(data = data, max_domain_column = max_domain_column, overall = overall, weight = FALSE)
3.
tidy_data(data, max_domain_column = max_domain_column, domain_names = domain_names, overall = overall, levels = c("h", "s", "l", "n", "x"))
2.
rob_traffic_light_rob2(data = data, tool = tool, rob_colours = rob_colours, psize = psize, overall = overall)
1.
rob_traffic_light(data = rob2, psize = 10, tool = "ROB2")

The docs say:

For example, a ROB2.0 dataset would have 8 columns (1 for study details, 5 for domain level judgments, 1 for overall judgements, and 1 for weights, in that order).

Here's my SessionInfo:

sessionInfo()
R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 18.04.4 LTS

Matrix products: default
BLAS:   /usr/lib/x86_64-linux-gnu/openblas/libblas.so.3
LAPACK: /usr/lib/x86_64-linux-gnu/libopenblasp-r0.2.20.so

locale:
 [1] LC_CTYPE=en_GB.UTF-8       LC_NUMERIC=C               LC_TIME=en_GB.UTF-8        LC_COLLATE=en_GB.UTF-8     LC_MONETARY=en_GB.UTF-8   
 [6] LC_MESSAGES=en_GB.UTF-8    LC_PAPER=en_GB.UTF-8       LC_NAME=C                  LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_GB.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
 [1] pwr_1.3-0        kableExtra_1.1.0 forcats_0.5.0    stringr_1.4.0    purrr_0.3.4      readr_1.3.1      tidyr_1.1.2      tibble_3.0.6     tidyverse_1.3.0 
[10] dplyr_1.0.4      ggplot2_3.3.3    robvis_0.3.0.900 metafor_2.5-75   Matrix_1.2-18    forester_0.2.0  

loaded via a namespace (and not attached):
 [1] httr_1.4.2        pkgload_1.1.0     jsonlite_1.7.2    viridisLite_0.3.0 here_1.0.1        modelr_0.1.6      assertthat_0.2.1  blob_1.2.1       
 [9] cellranger_1.1.0  yaml_2.2.1        remotes_2.2.0     sessioninfo_1.1.1 pillar_1.4.7      backports_1.1.9   lattice_0.20-41   glue_1.4.2       
[17] digest_0.6.27     rvest_0.3.5       colorspace_2.0-0  htmltools_0.5.1.1 pkgconfig_2.0.3   devtools_2.3.2    broom_0.5.6       haven_2.2.0      
[25] bookdown_0.20     patchwork_1.1.1   scales_1.1.1      webshot_0.5.2     processx_3.4.5    generics_0.1.0    farver_2.0.3      usethis_2.0.1    
[33] ellipsis_0.3.1    withr_2.4.1       cli_2.3.0         readxl_1.3.1      magrittr_2.0.1    crayon_1.4.1      memoise_2.0.0     evaluate_0.14    
[41] ps_1.5.0          fs_1.5.0          nlme_3.1-147      xml2_1.3.2        pkgbuild_1.2.0    rsconnect_0.8.16  tools_3.6.3       prettyunits_1.1.1
[49] hms_0.5.3         lifecycle_0.2.0   reprex_0.3.0      munsell_0.5.0     callr_3.5.1       compiler_3.6.3    systemfonts_1.0.1 rlang_0.4.10     
[57] grid_3.6.3        rstudioapi_0.13   labeling_0.4.2    rmarkdown_2.5     testthat_3.0.1    gtable_0.3.0      DBI_1.1.0         curl_4.3         
[65] R6_2.5.0          lubridate_1.7.9   gridExtra_2.3     knitr_1.31        mathjaxr_1.2-0    rprojroot_2.0.2   desc_1.2.0        stringi_1.5.3    
[73] Rcpp_1.0.6        vctrs_0.3.6       dbplyr_1.4.4      tidyselect_1.1.0  xfun_0.21        
earcanal commented 3 years ago

Removing Weight fixes this for me:

rob_traffic_light(data = rob2 %>% select(-Weight), psize=5, tool = "ROB2")
rob_traffic_light(data = robins %>% select(-Weight), psize=5, tool = "ROBINS-I")
mcguinlu commented 3 years ago

Hi @earcanal,

Thanks for your message - this is expected behaviour in the newest (0.3.0.900) version of robvis, and is documented as a breaking change in the ChangeLog:

There are now stricter conditions placed on the data provided to robvis based on the options specified, which also address some niche cases which were causing bugs. Informative messages are provided to users if the data is not compatible with their chosen options. Existing code may have to be updated - I am sorry for this, but this should make it easier to support long-term stability of the API

As rob_traffic_light() does not require weights to produce the plot, passing a dataframe with a weight column gives the error above.

This change forms part of a major overhaul of the codebase in 0.3.0.900 to a) address some silly decisions I made in setting the default values (e.g. setting weighted = TRUE in rob_summary()), b) standardise arguments across exisiting functions,, c) allow for handling of niche edge cases that was not possible under the old systme, and d) allow for substantially easier maintainece and addition of additional functionality in the future. While I appreciate that the changes may cause some issues for existing users, I made the decision that they were worth it for the long-term health of the project.

I'd be very interested to hear your feedback on the "informative messages" that accompany the error? Did you find them useful, and is there anything I could add or phrase differently to make it easier for users to know what is going on?

earcanal commented 3 years ago

Thanks for clarifying. I think the error message is clear, but it seems to conflict with the docs (see above) which still refer to 8 columns (including 'weights').

mcguinlu commented 3 years ago

Ah I see - I'll update the docs to accurately reflect the changes! Thanks @earcanal

mcguinlu commented 1 year ago

@earcanal - just confirming this has finally been addressed!

Thanks again for flagging the issue 😃