General Issue: Restructure Files for ADEG and ADVS Parameters

[bundfussr]

Background Information

The files derive_adeg_parms.R and derive_advs_parms.R should be restructured such that there is one file per "parameter". E.g., derive_param_qtc.R contains derive_param_qtc() and compute_qtc().
The test files should be restructured accordingly.

Definition of Done

derive_adeg_parms.R and derive_advs_parms.R and their test files are replaced.

[bms63]

@pharmaverse/admiral @pharmaverse/admiral_comm great issue to dig into the files of admiral and get familiar with some some of the functions. This is probably 1 hour of work and might produce some "uuummmm...moments" that we will need to discuss in the Pull Request.

[luenhchang]

@bms63 @jeffreyad
It seems that the task involves transferring both the function code and its documentation from the original R files to the new R files, as shown in the table below:

Original File	Function	New File
derive_adeg_params.R	derive_param_qtc	derive_param_qtc.R
derive_adeg_params.R	default_qtc_paramcd	derive_param_qtc.R
derive_adeg_params.R	compute_qtc	derive_param_qtc.R
derive_adeg_params.R	derive_param_rr	derive_param_rr.R
derive_adeg_params.R	compute_rr	derive_param_rr.R
------------------------	----------------------	---------------------
derive_advs_params.R	derive_param_map	derive_param_map.R
derive_advs_params.R	compute_map	derive_param_map.R
derive_advs_params.R	derive_param_bsa	derive_param_bsa.R
derive_advs_params.R	compute_bsa	derive_param_bsa.R
derive_advs_params.R	derive_param_bmi	derive_param_bmi.R
derive_advs_params.R	compute_bmi	derive_param_bmi.R

[jeffreyad]

@bms63 @jeffreyad It seems that the task involves transferring both the function code and its documentation from the original R files to the new R files, as shown in the table below:

Yes, and also change the test files test-derive_advs_params.R and test-derive_adeg_params.R the same way

[bms63]

Love this table.

@bundfussr and @manciniedoardo should we split the compute functions into separate files we sometimes do this and sometimes do not.

@luenhchang good to review this section as well as you are exploring admiral more

[bundfussr]

@bundfussr and @manciniedoardo should we split the compute functions into separate files we sometimes do this and sometimes do not.

I wouldn't put the compute functions into separate files because the derive and compute function are so closely connected.

[luenhchang]

@bms63 @jeffreyad @bundfussr

I have created the following new files:

1. R/derive_param_bmi.R
2. R/derive_param_bsa.R
3. R/derive_param_map.R
4. R/derive_param_qtc.R
5. R/derive_param_rr.R
6. tests/testthat/test-derive_param_bmi.R
7. tests/testthat/test-derive_param_bsa.R
8. tests/testthat/test-derive_param_map.R
9. tests/testthat/test-derive_param_qtc.R
10. tests/testthat/test-derive_param_rr.R

I haven’t pushed them yet.

Some questions:

• Does the test numbering in the test-*.R files always need to start at 1? For example, in test-derive_param_rr.R, the test headings currently begin at 5 as its original file:

"## Test 5: new observations are derived correctly ----"
"## Test 6: Message if no new records ---- "
Should these be renumbered to start at Test 1 and Test 2?

• Should I delete the original files to avoid redundancy? These include:
  derive_adeg_params.R
  derive_advs_params.R
  test-derive_adeg_params.R
  test-derive_advs_params.R

• Should the following R commands be run whenever files are created, deleted, or edited?
  styler::style_file()
devtools::document()
roxygen2::roxygenize('.', roclets = c('rd', 'collate', 'namespace'))

[bms63]

@bms63 @jeffreyad @bundfussr

I have created the following new files:

1. R/derive_param_bmi.R
2. R/derive_param_bsa.R
3. R/derive_param_map.R
4. R/derive_param_qtc.R
5. R/derive_param_rr.R
6. tests/testthat/test-derive_param_bmi.R
7. tests/testthat/test-derive_param_bsa.R
8. tests/testthat/test-derive_param_map.R
9. tests/testthat/test-derive_param_qtc.R
10. tests/testthat/test-derive_param_rr.R

I haven’t pushed them yet.

Some questions:

• Does the test numbering in the test-*.R files always need to start at 1? For example, in test-derive_param_rr.R, the test headings currently begin at 5 as its original file:

Yes! If you are restructuring tests or updating old ones it is good to re-number the Tests. You can install this addin from github pharmaverse4devs to help with renumbering

https://pharmaverse.github.io/admiraldev/dev/articles/unit_test_guidance.html#addin-pharmaverse4devsformat_test_that_file

"## Test 5: new observations are derived correctly ----" "## Test 6: Message if no new records ---- " Should these be renumbered to start at Test 1 and Test 2?

Yes! Use the addin as it is super quick and easy. We like our bureaucracy in admiral!

• Should I delete the original files to avoid redundancy? These include:
  derive_adeg_params.R
  derive_advs_params.R
  test-derive_adeg_params.R
  test-derive_advs_params.R

Yes please remove these files - good to run local devtools::check() but the GHA will also pick up any issues

• Should the following R commands be run whenever files are created, deleted, or edited?
  styler::style_file()
devtools::document()
roxygen2::roxygenize('.', roclets = c('rd', 'collate', 'namespace'))

The first two - I think the third is needed if multiple people are using different versions of the roxygen and the GHAs checks get confused and recommends it. Maybe others know why @pharmaverse/admiral

[bms63]

Hi @luenhchang checking in on the progress for this issue?

We have a release planned for January 15th and would like to include this update.

[luenhchang]

@bms63
I updated the files and pushed the changes on 23 Dec 2024. I don't know if you can see the changes. The most recent git bash commands in my record:

git add .
git commit -m "Deleted unused test and R scripts, formatted 5 test_that test files"
#[feature/my_first_fcn 76def67c2] Deleted unused test and R scripts, formatted 5 test_that test #files
# 20 files changed, 131 insertions(+), 3268 deletions(-)
# delete mode 100644 R/derive_adeg_params.R
# delete mode 100644 R/derive_advs_params.R
# delete mode 100644 tests/testthat/test-derive_adeg_params.R
# delete mode 100644 tests/testthat/test-derive_advs_params.R

git push origin feature/my_first_fcn

[bms63]

Awesome!! Can you set up a pull request?