OHDSI / CohortDiagnostics

An R package for performing various cohort diagnostics.
https://ohdsi.github.io/CohortDiagnostics
41 stars 48 forks source link

Cohort Diagnostics organization and export underlying functions #986

Closed mdlavallee92 closed 1 year ago

mdlavallee92 commented 1 year ago

Purpose

Goal of pull request is to provide documentation to key underlying functions of cohort diagnostics that sit underneath the global execute executeDiagnostics. Also helped organized functions to provide more UI consistency. Create Unit tests for newly exported functions (TODO).

Tasks

1) Unify terminology for cohort diagnostic functions:

get (single cohort) => batch (multiple cohorts) => execute (global run) Function signatures were changed to match these key verbs.

2) make function arguments consistent

some examples: cohorts input changed to cohortDefinitionSet (since this is the input in the execute) covariateSettings changed to temporalCovariateSettings

Also organized signatures: batch_fn args example: connection schemas(cdm, vocab, temp, cohort) cohortTable cohortDefinitionSet cdmVersion (if needed) Export (databaseId, exportFoldr, minCellCount) ->match makeDataExportable Batch (instantiatedCohorts, recordKeepingFile, incremental) -> match recordTasksDone for incremental mode remaining function options

3) Unit Tests

In progress 🏗️

Work Zone 🚧

Files where work was done: 1) Cohort Counts (CohortLevelDiagnostics.R) 2) Inclusion Rules (InclusionRules.R) 3) Concept Sets (ConceptSets.R) 4) Time Series (TimeSeries.R) 5) Visit Context (VisitContext.R) 6) Incidence Rates (IncidenceRates.R) 7) Cohort Overlap (CohortRelationship.R) 8) Temporal Characterization (CohortCharacterizationDiagnostics.R)
9) executeDiagnostics (RunDiagnostics.R) -> the global execute function

mdlavallee92 commented 1 year ago

@azimov for the unit tests can we create the cohort definitions once in the setup file and use them for all of the tests? Is there a particular reason the cohorts are generated across multiple tests?

Also I changed the functions so that only those that actually run in batch are called batch (removed the export). and functions that are just iterative wrappers are called generate. Let me know if that is ok

azimov commented 1 year ago

@azimov for the unit tests can we create the cohort definitions once in the setup file and use them for all of the tests? Is there a particular reason the cohorts are generated across multiple tests?

I'd need to check each individual test to be certain but this is almost certainly a handover from when this package also generated the cohorts in place of CohortGenerator. If you want to change this feel free, it would be a big improvement and I'd certainly try to structure your tests this way if you can.

azimov commented 1 year ago

@mdlavallee92 you should have permissions to create the branch on the OHDSI github rather than your fork. Doing this you'll be able to run the checks without them failing. Alternatively you can set your github secrets to the OHDSI ones (I think you'll need to generate a github PAT to download packages).

mdlavallee92 commented 1 year ago

@azimov closing since this is too out of date. will start fresh