Prevent reorder_studies from dropping rows

sangeetabhatia03 commented 2 weeks ago

This PR fixes #73 where reorder_studies was causing rows to be dropped when the column used to order studies contained NA. To fix this, I have edited the code so that NA values are treated as factor levels. Additionally,

I have modified the code so that reorder_studies accepts as an argument the column name to be used for reordering.
I have modified forest_plot to set explicit values for NAs in columns mapped to colour or shape aesthetic.

To test this PR, do the following

ebola <- load_epidata("ebola")
params <- ebola$params
si <- params[params$parameter_type %in% "Human delay - serial interval", ]
## Retain only the first six rows so that fix is easier to see
si <- head(si)
si$population_country[1] <- NA
sio <- reorder_studies(param_pm_uncertainty(si))
## sio should also have 6 rows
dim(sio)
## Also check the plots
forest_plot_serial_interval(si, col_by = "population_country")

Checklist:

[ X] I have added new dependencies in DESCRIPTION
[ X] I have bumped the version number in DESCRIPTION
[ X] I have updated the NEWS.md
[ X] I have added unit tests using testthat
[ X] I have checked that the package can install locally using devtools::install_local("<path-to-epireview>")
[ X] I have added documentation where required
[ X] My documentation includes examples of how to use the function
[ NA] If necessary, I have updated priority-pathogens

CosmoNaught commented 2 weeks ago

Tested package integrity with check() and test().

Passing check()

── R CMD check results ────────────────────────────────────────────────────────────── epireview 1.2.8 ────
Duration: 39.6s

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

on check() however worth pointing out we got a few errors with the way that some tests are written/handled

ℹ Testing epireview
✔ | F W  S  OK | Context
✔ |   3      9 | add_unique_labels                                                                        
──────────────────────────────────────────────────────────────────────────────────────────────────────────
Warning (test_add_unique_labels.R:40:3): pretty article labels work
There are 1 articles with missing first author surname and first author first name.
Backtrace:
    ▆
 1. ├─testthat::expect_warning(pretty_article_label(articles2, mark_multiple = mark_multiple)) at test_add_unique_labels.R:40:3
 2. │ └─testthat:::expect_condition_matching(...)
 3. │   └─testthat:::quasi_capture(...)
 4. │     ├─testthat (local) .capture(...)
 5. │     │ └─base::withCallingHandlers(...)
 6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 7. └─epireview::pretty_article_label(articles2, mark_multiple = mark_multiple)

Warning (test_add_unique_labels.R:54:3): pretty article labels work
There are 1 articles with missing first author surname and first author first name.
Backtrace:
    ▆
 1. ├─testthat::expect_warning(pretty_article_label(articles2, mark_multiple = mark_multiple)) at test_add_unique_labels.R:54:3
 2. │ └─testthat:::expect_condition_matching(...)
 3. │   └─testthat:::quasi_capture(...)
 4. │     ├─testthat (local) .capture(...)
 5. │     │ └─base::withCallingHandlers(...)
 6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 7. └─epireview::pretty_article_label(articles2, mark_multiple = mark_multiple)

Warning (test_add_unique_labels.R:54:3): pretty article labels work
There are 1 articles with missing year of publication.
Backtrace:
    ▆
 1. ├─testthat::expect_warning(pretty_article_label(articles2, mark_multiple = mark_multiple)) at test_add_unique_labels.R:54:3
 2. │ └─testthat:::expect_condition_matching(...)
 3. │   └─testthat:::quasi_capture(...)
 4. │     ├─testthat (local) .capture(...)
 5. │     │ └─base::withCallingHandlers(...)
 6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 7. └─epireview::pretty_article_label(articles2, mark_multiple = mark_multiple)
──────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ |          8 | assertions                                                                               
✔ |          5 | data_load_and_prep                                                                       
⠏ |          0 | doi_included                                                                             No duplicate covidence ids found
No duplicate covidence ids found
Data loaded for lassa
✔ |          3 | doi_included                                                                             
✔ |          6 | get_parameter                                                                            
⠏ |          0 | make_unique_id                                                                           No duplicate covidence ids found
No duplicate covidence ids found
Data loaded for lassa
✔ |          3 | make_unique_id                                                                           
✔ |          4 | reorder_studies                                                                          
⠙ |   2     20 | utils                                                                                    No duplicate covidence ids found
No duplicate covidence ids found
Data loaded for ebola
✔ |   2     24 | utils                                                                                    
──────────────────────────────────────────────────────────────────────────────────────────────────────────
Warning (test_utils.R:116:3): reparam_gamma correctly reparameterizes the data frame
No data found for  ebola
Backtrace:
    ▆
 1. └─epireview::load_epidata("ebola") at test_utils.R:116:3
 2.   └─epireview::load_epidata_raw(pathogen, "outbreak") at epireview/R/load_epidata.R:73:3

Warning (test_utils.R:116:3): reparam_gamma correctly reparameterizes the data frame
No outbreaks information found for  ebola
Backtrace:
    ▆
 1. └─epireview::load_epidata("ebola") at test_utils.R:116:3
──────────────────────────────────────────────────────────────────────────────────────────────────────────
⠏ |          0 | add-qa                                                                                   No duplicate covidence ids found
No duplicate covidence ids found
Data loaded for ebola
⠋ |          1 | add-qa                                                                                   1 articles have all NAs for QA questions; please check that
         this is expected. QA score for these articles is set to NA.
✔ |         12 | add-qa
⠏ |          0 | filter_df_for_metamean                                                                   No duplicate covidence ids found
No duplicate covidence ids found
Data loaded for lassa
⠋ |          1 | filter_df_for_metamean                                                                   parameter_unit is missing but parameter_value is present.
            Rows with non-NA parameter_value and NA parameter_unit will be
            removed.
✔ |         16 | filter_df_for_metamean

══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════
Duration: 1.8 s

[ FAIL 0 | WARN 5 | SKIP 0 | PASS 90 ]

Regarding the code-chunk I can confirm the code executes appropriately however warnings appear maybe @thomrawson or @sangeetabhatia03 can advise if expected

Warning messages:
1: In load_epidata_raw(pathogen, "outbreak") : No data found for  ebola
2: In load_epidata("ebola") : No outbreaks information found for  ebola

Warning message:
In invert_inverse_params(reparam_gamma(df)) : No parameters to invert.

However dimensionality is maintained (6) and the plot works and looks good. Please advise, happy to approve otherwise.

CosmoNaught commented 2 weeks ago

My main concern are these warnings

Warning messages:
1: In load_epidata_raw(pathogen, "outbreak") : No data found for  ebola
2: In load_epidata("ebola") : No outbreaks information found for  ebola

Let me know if I'm not understanding something. If these warnings are expected then we need something a bit more verbose dare I say. As presently shown the implications of this "missingness" isn't highlighted to the user.

sangeetabhatia03 commented 2 weeks ago

Thanks Cosmo, you are right that these are not good warning messages. In fact, everyone who has used epireview thinks that Ebola data load has failed because of these messages. We have two open issues (#21, #57) to deal with these, and @tristan-myles is working on improving the messages as well as better error handling.

CosmoNaught commented 2 weeks ago

Thanks Cosmo, you are right that these are not good warning messages. In fact, everyone who has used epireview thinks that Ebola data load has failed because of these messages. We have two open issues (#21, #57) to deal with these, and @tristan-myles is working on improving the messages as well as better error handling.

Ah brilliant, gad to hear that. In that case happy to approve. I'll let @thomrawson merge once he has taken a look and he's happy with it. On my side code integrity is good other than poor error/warning handling.

mrc-ide / epireview

Prevent reorder_studies from dropping rows #100