2nd Code Review

[JSchoenbachler]

This issue will track the 2nd code review for the phers package.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/phers.R#L81-L82

Spacing

  demos[, person_ID := as.character(person_ID)]
  phecodes[, person_ID := as.character(person_ID)]

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/phers.R#L96

You should use setnames to just rename the column since you don't use the ID column after this.

[JSchoenbachler]

Instead of 2 R scripts with one function in each (the map_*.R scripts), you should make a utils.R script that contains both of those functions. A utils.R script is great for holding functions that don't really have another script they would logically fit in with and/or unexported functions you use within the package but don't care for others to have access to.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_ICD_to_phecode.R#L1-L3

You only need to import packages and functions once. Keep them in the phers.R script and remove here.

[jakejh]

And I don't think .N needs to be imported, since it's not a function.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_ICD_to_phecode.R#L9

Never use :: within a package, but also if this variable exists in the phers package, it will be accessible without using the ::.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_ICD_to_phecode.R#L16-L17

Spacing

  phecodes = merge(ICDs, ICDPhecodeMap,
                   by = c('ICD', 'flag'))[, .(person_ID, phecode)]

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_disease_to_phecode.R#L1-L3

Imports, see previous comment.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_disease_to_phecode.R#L14-L15

See previous comment about ::.

[jakejh]

And I would prefer a line break after merge(, to avoid excessive indentation.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_disease_to_phecode.R#L23

Use | in your i argument instead of double-bracketing. Like this:

  diseaseHPOMapSub = diseaseHPOMap[db_name == dbName | disease_ID %in% diseaseIDs]

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/R/map_disease_to_phecode.R#L24-L27

More spacing

  diseasePhecodeMap =merge(diseaseHPOMapSub,
                           HPOPhecodeMap[, !c('HPO_term_name')], by = 'term_ID')
  diseasePhecodeMap = unique(diseasePhecodeMap[phecode != ''][, c('disease_ID',
                                                               'phecode')])

[JSchoenbachler]

Like Jake is saying, look at some of your merge statements and any other line to see about being a little more tactical in your use of line breaks to clean things up.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/tests/testthat/setup.R#L5

Few things:

• Never hard code the amount of cores. Even at 2 cores, some cloud instances and the like will check and/or test the package in small containers that might not have the ability to do something like this. Use a function to detect the amount of cores and use like 50% or 75% of that.
• In addition, tests and vignettes should never use parallel functions. Use registerDoSEQ() here instead.

[JSchoenbachler]

Fix up the spacing in general all around. To specifically call out a file though, look at the setup.R script

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/tests/testthat/test-phers.R#L1-L15

Combine these into a single test function, and use the snapshot method instead of the way you are doing it in lines 5-7.

[JSchoenbachler]

Noticed a few spacing issues in test-phers.R.

[JSchoenbachler]

Based on previous comment, when you move the map script functions into a single utils.R script, update the test files accordingly.

[JSchoenbachler]

For both of the map tests:

• Check spacing
• Add something that actually tests the function output rather than just error handling.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/tests/testthat/setup.R#L28-L35

Make these into data files in the data folder instead of making them this way.

[JSchoenbachler]

https://github.com/hugheylab/phers/blob/adc2ca85fe38a92bf4795ac4a946e15c312290e2/vignettes/generate_phers.Rmd#L22

See above comment about registerDoParallel.

[jakejh]

As a naming convention, I now prefer get as a function prefix instead of calc.