Intuitive names for parameter types

kellymccain28 commented 1 month ago

@CarmenTamayo and I worked on this this afternoon!

The parameter type values were clunky and long, inconsistent between parameter datasets, and not intuitive for someone to easily filter the parameter dataset. See this issue.

We've made a lookup table based on all of the unique parameter types that exist now and have used that to make parameter_type_short that is a more consistently labelled column for parameter type. We have also updated the marburg and lassa params datasets to incorporate the 'other' delays in the parameter_type column.

Checklist:

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

sangeetabhatia03 commented 3 weeks ago

@kellymccain28 can you write test(s) for your changes? I suspect the code as written might not work because I detected a typo in a function call.

kellymccain28 commented 2 weeks ago

Thanks Sangeeta! I've made those fixes and added some tests (though haven't really written tests before, so if you have suggestions for other things to test for please let me know!).

sangeetabhatia03 commented 1 week ago

@kellymccain28 I am afraid this still won't work as I don't see the variable param_name being passed to the relevant function. I will edit it now and see if it works.

sangeetabhatia03 commented 1 week ago

Hi @CosmoNaught if you have a moment, would be grateful for your review. Brief background: the datafiles have a column called "parameter_type" which at the moment is really clunky because the values are long and filled with spaces in weird places. We have therefore created a new column called "parameter_type_short", this is being done as part of load_epidata_raw. The mapping of long to short column names is in param_name.csv. I am reading this file in the function short_parameter_type but unsure if there is a cleaner way to do this. Any other comments/feedback welcome!

CosmoNaught commented 1 week ago

@sangeetabhatia03

failing on test()

Failure (test-get_key_columns.R:283:3): get_key_columns returns all columns when all_columns = TRUE
dim(df_all) (`actual`) not identical to c(374L, 64L) (`expected`).

  `actual`: 374 65
`expected`: 374 64
──────────────────────────────────────────────────────────────────────────────────────────────────────────

══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════
Duration: 3.3 s

── Failed tests ──────────────────────────────────────────────────────────────────────────────────────────
Failure (test-get_key_columns.R:283:3): get_key_columns returns all columns when all_columns = TRUE
dim(df_all) (`actual`) not identical to c(374L, 64L) (`expected`).

  `actual`: 374 65
`expected`: 374 64

[ FAIL 1 | WARN 5 | SKIP 0 | PASS 157 ]

I have fixed this issue locally by assigning the correct dimensionality to the problematic line of code here https://github.com/mrc-ide/epireview/blob/e2f5a414d8b0bcbdb26e2a8ca5fc77f66dc17e2e/tests/testthat/test-get_key_columns.R#L283 -- here it is set to 64, however it should be 65.

See here for develop branch value https://github.com/mrc-ide/epireview/blob/85f1537826fc4b7ad810656238ef37b60e82e7aa/tests/testthat/test-get_key_columns.R#L283 as this is unchanged and as you mentioned we've expanded the col dims.

Please confirm and I can push a patch.

CosmoNaught commented 1 week ago

Passing check()

── R CMD check results ──────────────────────────────────────────────────────── epireview 1.2.10 ────
Duration: 40.7s

❯ checking R code for possible problems ... NOTE
  load_epidata: no visible global function definition for ‘read.csv2’
  Undefined global functions or variables:
    read.csv2
  Consider adding
    importFrom("utils", "read.csv2")
  to your NAMESPACE file.

0 errors ✔ | 0 warnings ✔ | 1 note ✖

Passing test()


══ Results ══════════════════════════════════════════════════════════════════════════════════════════
Duration: 3.3 s

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

Fixed issue with https://github.com/mrc-ide/epireview/pull/76/commits/8a7f4a51025f014669c2a57eb98704b53b3df5d0

CosmoNaught commented 1 week ago

r$> roxygen2::roxygenise()
ℹ Loading epireview
✖ load_epidata_raw.R:284: @examples requires a value.

https://github.com/mrc-ide/epireview/pull/76/commits/d43207c1a2a8ec29e867fc52b050e983ac2ffd99 fixes missing function reference

mrc-ide / epireview

Intuitive names for parameter types #76