Darwin sprint - #1174
Draft
ablack3 wants to merge 282 commits into
Draft
Conversation
…s not already exist. Skip results datamode test if SKIP_DB_TESTS is TRUE.
…ept_ids is created in the function, Also I modified the insert statement for the concept_ids table to ensure that only concept ids which are not already present in the table are addded.
…function is run twice, the table contains no duplicates, hence the output from each run should be identical.
runInclusionStatistics
Merge branch 'darwin_sprint' into useExternalConceptCounts # Conflicts: # DESCRIPTION # NAMESPACE # R/CohortLevelDiagnostics.R # R/ConceptSetUtils.R # R/hold/executeDiagnostics.R # R/runBreakdownIndexEvents.R # R/utils.R # inst/sql/sql_server/CreateConceptCountTable.sql # man/executeDiagnostics.Rd # tests/testOracle.R # tests/testPostgres.R # tests/testRedshift.R # tests/testSqlServer.R # tests/testthat/setup.R # tests/testthat/test-2-againstCdm.R
Merge branch 'darwin_sprint' into useExternalConceptCounts # Conflicts: # man/createConceptCountsTable.Rd # man/executeDiagnostics.Rd
Fix r-cmd check
…n/Date) Co-authored-by: Cursor <cursoragent@cursor.com>
…ithout a warning.
There was a problem hiding this comment.
Pull request overview
WIP refactor and feature expansion aimed at CohortDiagnostics v4.0.0, including new diagnostics runners, new utilities (concept counts, safe max), and in-package Shiny Diagnostics Explorer modules (removing reliance on OhdsiShinyModules), plus updated docs/CI/test scaffolding.
Changes:
- Adds/updates core diagnostics runners (e.g., visit context, inclusion stats, time series, incidence rate) and supporting utilities (concept counts table, logging plot, argument validation).
- Vendors Shiny Diagnostics Explorer modules into this package and updates app wiring/config/migrations references.
- Updates documentation (many new Rd pages), fixtures, and GitHub Actions configuration for testing/build.
Reviewed changes
Copilot reviewed 144 out of 183 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/hold/test-3-Characterization.R | Updates characterization test setup to create/generate cohort tables before running diagnostics. |
| tests/testthat/hold/test-2-againstCdm.R | Adds a (hold) integration-style diagnostics test over test servers. |
| tests/testthat/helper.R | Updates subset definition helpers and adds DuckDB SQL-test fixture + CDM helpers. |
| tests/testthat/_snaps/shinytest2-DiagnosticsExplorer/DiagnosticsExplorer-001.json | Adds shinytest2 snapshot for Diagnostics Explorer. |
| tests/testthat.R | Adds standard testthat entrypoint for package checks. |
| tests/testSqlite.R | Removes DB-specific test runner script. |
| tests/testSqlServer.R | Removes DB-specific test runner script. |
| tests/testRedshift.R | Removes DB-specific test runner script. |
| tests/testPostgres.R | Removes DB-specific test runner script. |
| tests/testOracle.R | Removes DB-specific test runner script. |
| man/safeMax.Rd | Documents new safeMax() helper. |
| man/runVisitContext.Rd | Documents new runVisitContext() export. |
| man/runTimeSeries.Rd | Documents runTimeSeries() API. |
| man/runResolvedConceptSets.Rd | Documents runResolvedConceptSets() API. |
| man/runOrphanConcepts.Rd | Documents runOrphanConcepts() API. |
| man/runInclusionStatistics.Rd | Documents runInclusionStatistics() API. |
| man/runIncludedSourceConcepts.Rd | Documents runIncludedSourceConcepts() API. |
| man/runIncidenceRate.Rd | Documents runIncidenceRate() API. |
| man/runCohortTimeSeriesDiagnostics.Rd | Removes deprecated time series diagnostics doc. |
| man/runCohortCharacterization.Rd | Documents runCohortCharacterization() API. |
| man/runBreakdownIndexEvents.Rd | Documents runBreakdownIndexEvents() API. |
| man/reexports.Rd | Adds reexport doc for getCohortCounts. |
| man/plotLogFile.Rd | Documents plotLogFile() API. |
| man/launchDiagnosticsExplorer.Rd | Removes deprecated arg docs for Diagnostics Explorer launcher. |
| man/getResultsDataModelSpecifications.Rd | Documents new tableName parameter. |
| man/getDefaultCovariateSettings.Rd | Updates source reference + value section. |
| man/getConceptCountsTableName.Rd | Documents getConceptCountsTableName(). |
| man/getCohortCounts.Rd | Removes local getCohortCounts() doc (now reexported). |
| man/getCdmDataSourceInformation.Rd | Updates roxygen source file reference. |
| man/executeDiagnostics.Rd | Updates executeDiagnostics docs (incrementalFolder/conceptCountsTable/etc). |
| man/createConceptCountsTable.Rd | Documents createConceptCountsTable(). |
| man/checkInputFileEncoding.Rd | Updates source reference for encoding checker doc. |
| man/checkArg.Rd | Documents new internal checkArg() helper. |
| man/allCovariateSettings.Rd | Documents internal covariate settings validator. |
| man-roxygen/minCharacterizationMean.R | Adds roxygen template for min characterization mean. |
| man-roxygen/databaseId.R | Adds roxygen template for databaseId. |
| man-roxygen/connectionDetails.R | Adds roxygen template for connectionDetails/connection. |
| man-roxygen/cohortDefinitionSet.R | Adds cohortDefinitionSet roxygen template. |
| man-roxygen/atlasId.R | Removes unused atlasId template. |
| man-roxygen/RecordKeepingFile.R | Adds recordKeepingFile template. |
| man-roxygen/OracleTempSchema.R | Removes deprecated oracleTempSchema template. |
| man-roxygen/ModeAndDetails.R | Removes large legacy template block. |
| man-roxygen/MinCellCount.R | Adds minCellCount template. |
| man-roxygen/InstantiatedCohorts.R | Adds instantiatedCohorts template. |
| man-roxygen/Incremental.R | Adds incremental/incrementalFolder template. |
| man-roxygen/ExportFolder.R | Adds exportFolder template. |
| man-roxygen/DatabaseIds.R | Adds databaseId param template alongside databaseIds. |
| man-roxygen/Connection.R | Simplifies connection template. |
| man-roxygen/CohortSetReference.R | Updates details template for cohortDefinitionSet. |
| man-roxygen/BatchSize.R | Adds batchSize template. |
| inst/testCases/testSubjectCountsNoVisits/getVisitContext-patientData.json | Adds visit-context fixture input JSON (no-visits scenario). |
| inst/testCases/testSubjectCountsNoVisits/getVisitContext-cohort.csv | Adds visit-context fixture cohort CSV (no-visits scenario). |
| inst/testCases/testSubjectCountsNoVisits/expectedResult.csv | Adds expected-result header for no-visits scenario. |
| inst/testCases/testSubjectCountsDates/getVisitContext-patientData.json | Adds visit-context fixture input JSON (dates scenario). |
| inst/testCases/testSubjectCountsDates/getVisitContext-cohort.csv | Adds visit-context fixture cohort CSV (dates scenario). |
| inst/testCases/testSubjectCountsDates/expectedResult.csv | Adds expected visit-context results (dates scenario). |
| inst/testCases/testSubjectCounts/getVisitContext-patientData.json | Adds visit-context fixture input JSON (basic scenario). |
| inst/testCases/testSubjectCounts/getVisitContext-cohort.csv | Adds visit-context fixture cohort CSV (basic scenario). |
| inst/testCases/testSubjectCounts/expectedResult.csv | Adds expected visit-context results (basic scenario). |
| inst/sql/sql_server/includedSourceConcepts.sql | Adds inline optimization notes. |
| inst/sql/sql_server/CreateTimeSeriesCohortTable.sql | Adds new SQL Server time series cohort temp table creation script. |
| inst/sql/sql_server/CreateConceptIdTable.sql | Removes SQL Server concept-id temp table script. |
| inst/sql/sql_server/CreateConceptCountTable.sql | Adds removeCurrentTable flag + vocabulary_version column population. |
| inst/sql/sql_server/CreateCohortTable.sql | Removes SQL Server cohort table creation script. |
| inst/sql/sql_server/CohortCounts.sql | Removes SQL Server cohort-counts query script. |
| inst/shiny/DiagnosticsExplorer/ui.R | Switches UI calls to in-package module functions. |
| inst/shiny/DiagnosticsExplorer/server.R | Switches server call to in-package cohortDiagnosticsServer. |
| inst/shiny/DiagnosticsExplorer/modules/helpers-migrations.R | Adds migration-table lookup helpers for Shiny. |
| inst/shiny/DiagnosticsExplorer/modules/helpers-logo.R | Adds logo helper for Shiny UI. |
| inst/shiny/DiagnosticsExplorer/modules/helpers-getHelp.R | Adds help-file locator helper. |
| inst/shiny/DiagnosticsExplorer/modules/helpers-emptyPlotly.R | Adds plotly empty plot + text wrapping helpers. |
| inst/shiny/DiagnosticsExplorer/modules/helpers-elements.R | Adds tooltip + reactable CSV download UI helpers. |
| inst/shiny/DiagnosticsExplorer/modules/components-inputselection.R | Adds input selection component module. |
| inst/shiny/DiagnosticsExplorer/modules/components-helpInfo.R | Adds help/info box component. |
| inst/shiny/DiagnosticsExplorer/modules/component-tableSelect.R | Adds modal-based table selection module. |
| inst/shiny/DiagnosticsExplorer/modules/cohort-diagnostics-visitContext.R | Vendors visit-context module UI/server + SQL-backed queries. |
| inst/shiny/DiagnosticsExplorer/modules/cohort-diagnostics-orphanConcepts.R | Vendors orphan concepts module UI/server. |
| inst/shiny/DiagnosticsExplorer/modules/cohort-diagnostics-main-ui.R | Vendors main diagnostics UI (tabs + sidebar controls) into package. |
| inst/shiny/DiagnosticsExplorer/modules/cohort-diagnostics-indexEventBreakdown.R | Vendors index event breakdown module UI/server. |
| inst/shiny/DiagnosticsExplorer/modules/cohort-diagnostics-inclusionRules.R | Vendors inclusion rules module UI/server. |
| inst/shiny/DiagnosticsExplorer/global.R | Updates Shiny global wiring: dplyr availability, module sourcing, migrations/spec paths. |
| inst/cohort-diagnostics-www/visitContext.html | Adds documentation HTML for visit context report. |
| inst/cohort-diagnostics-www/timeDistribution.html | Adds documentation HTML for time distributions report. |
| inst/cohort-diagnostics-www/orphanConcepts.html | Adds/updates documentation HTML for orphan concepts report. |
| inst/cohort-diagnostics-www/indexEventBreakdown.html | Adds documentation HTML for index event breakdown report. |
| inst/cohort-diagnostics-www/inclusionRuleStats.html | Adds documentation HTML for inclusion rule stats report. |
| inst/cohort-diagnostics-www/incidenceRate.html | Adds documentation HTML for incidence rate report. |
| inst/cohort-diagnostics-www/conceptsInDataSource.html | Adds documentation HTML for concepts-in-data-source report. |
| inst/cohort-diagnostics-www/conceptSetDiagnostics.html | Adds documentation HTML for concept set diagnostics report. |
| inst/cohort-diagnostics-www/compareCohortCharacterization.html | Adds documentation HTML for compare characterization report. |
| inst/cohort-diagnostics-www/cohortOverlap.html | Adds documentation HTML for cohort overlap report. |
| inst/cohort-diagnostics-www/cohortCounts.html | Adds documentation HTML for cohort counts report. |
| inst/cohort-diagnostics-www/cohortCharacterization.html | Adds documentation HTML for cohort characterization report. |
| inst/cohort-diagnostics-www/cohort-diagnostics.html | Adds top-level Cohort Diagnostics HTML help page. |
| inst/cohort-diagnostics-ref/migrations.csv | Adds migrations reference CSV used by Shiny/data migrator. |
| inst/cohort-diagnostics-ref/Table1SpecsLong.csv | Adds table-1 specification reference. |
| extras/tests/TestLaunchDiagnosticsExplorer.R | Adds script to smoke-test launching Diagnostics Explorer from minimal SQLite. |
| extras/addingNewDiagnostics.Rmd | Adds developer guide draft for adding new diagnostics. |
| extras/CDM-interface.R | Adds exploratory notes for CDM interface usage. |
| R/saveDatabaseMetaData.R | Switches metadata export to exportDataToCsv(). |
| R/runVisitContext.R | Adds visit context SQL + export runner with incremental support. |
| R/runResolvedConceptSets.R | Adds resolved concept sets instantiation/export. |
| R/runInclusionStatistics.R | Adds inclusion statistics export flow via CohortGenerator. |
| R/runIncludedSourceConcepts.R | Adds included source concepts extraction/export. |
| R/runIncidenceRate.R | Refactors incidence rate runner and export logic. |
| R/plotLogFile.R | Adds log parsing + ggplot timing visualization. |
| R/getCdmDataSourceInformation.R | Uses safeMax and corrects DatabaseConnector qualifier. |
| R/exportConceptInformation.R | Switches concept export to exportDataToCsv(). |
| R/checkArg.R | Adds central argument validation utility. |
| R/Shiny.R | Removes OhdsiShinyModules install/version gating; adds SQLite indexing in merge step. |
| R/Shared.R | Removes legacy hasData() definition (moved elsewhere or replaced). |
| R/ResultsDataModel.R | Adds filtering option, suppresses upload warnings, adds migration messaging option. |
| R/Private.R | Removes large legacy private helpers block. |
| R/Incremental.R | Uses vapply, fixes key indexing, normalizes readr guess_max, adds parentJson tracking. |
| R/ConceptCountsTable.R | Adds concept counts table creation + versioned naming helper. |
| R/CohortLevelDiagnostics.R | Replaces local getCohortCounts() with reexport of CohortGenerator’s function. |
| R/CohortDiagnostics.R | Minor whitespace change. |
| NAMESPACE | Adds new exports and CohortGenerator import for getCohortCounts. |
| DESCRIPTION | Bumps to 4.0.0; updates dependencies/suggests and roxygen note. |
| CohortDiagnostics.Rproj | Enables devtools usage. |
| .github/workflows/R_CMD_check_sqlite_only.yaml | Sets TESTTHAT_CPUS and JDBC drivers folder env var. |
| .github/workflows/R_CMD_check_Hades.yaml | Sets JDBC drivers folder env var. |
| .Rbuildignore | Ignores work/ and sql/ paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
25
| getIncidenceRate <- function(connection = NULL, | ||
| cohortDatabaseSchema, | ||
| cohortTable, | ||
| cdmDatabaseSchema, | ||
| vocabularyDatabaseSchema = cdmDatabaseSchema, | ||
| cdmVersion = 5, | ||
| tempEmulationSchema = tempEmulationSchema, | ||
| firstOccurrenceOnly = TRUE, | ||
| washoutPeriod = 365, | ||
| cohortId) { |
There was a problem hiding this comment.
tempEmulationSchema = tempEmulationSchema is self-referential, so the default value will error when the caller doesn’t supply tempEmulationSchema. Use a real default (e.g., getOption('sqlRenderTempEmulationSchema') or NULL) and pass that through to SqlRender/DatabaseConnector calls.
| concept_subjects | ||
| {@table_is_temp} ? { | ||
| INTO @concept_counts_table | ||
| INTO @work_database_schema.@concept_counts_table |
There was a problem hiding this comment.
The @table_is_temp branch still writes INTO @work_database_schema.@concept_counts_table, which creates a permanent table (or fails) instead of a SQL Server temp table. This also conflicts with the earlier temp-table drop logic (tempdb..@concept_counts_table). For the temp-table case, write INTO @concept_counts_table (no schema) and keep schema-qualified naming only for non-temp tables.
Suggested change
| INTO @work_database_schema.@concept_counts_table | |
| INTO @concept_counts_table |
| checkmate::assertTRUE(DatabaseConnector::dbIsValid(arg)) | ||
|
|
||
| } else if (argName %in% c("tempEmulationSchema", "cdmDatabaseSchema", "vocabularyDatabaseSchema", "cohortDatabaseSchema")) { | ||
| checkmate::assertCharacter(arg, len = 1, pattern = "^[A-Za-z][A-Za-z0-9_]*$", null.ok = TRUE) |
There was a problem hiding this comment.
This schema validation regex rejects common supported schema formats documented elsewhere in the package (e.g., SQL Server scratch.dbo). Update the pattern to allow dots (and potentially other supported characters), e.g. permitting database.schema forms so valid inputs aren’t blocked.
Suggested change
| checkmate::assertCharacter(arg, len = 1, pattern = "^[A-Za-z][A-Za-z0-9_]*$", null.ok = TRUE) | |
| checkmate::assertCharacter(arg, len = 1, pattern = "^[A-Za-z][A-Za-z0-9_]*(\\.[A-Za-z][A-Za-z0-9_]*)*$", null.ok = TRUE) |
Comment on lines
+13
to
+41
| debugonce(executeDiagnostics) | ||
| firstTime <- system.time( | ||
|
|
||
| executeDiagnostics( | ||
| cohortDefinitionSet = server$cohortDefinitionSet, | ||
| connectionDetails = server$connectionDetails, | ||
| cdmDatabaseSchema = server$cdmDatabaseSchema, | ||
| vocabularyDatabaseSchema = server$vocabularyDatabaseSchema, | ||
| tempEmulationSchema = server$tempEmulationSchema, | ||
| cohortDatabaseSchema = server$cohortDatabaseSchema, | ||
| cohortTable = server$cohortTable, | ||
| cohortIds = server$cohortIds, | ||
| exportFolder = exportFolder, | ||
| databaseId = nm, | ||
| runInclusionStatistics = TRUE, | ||
| runBreakdownIndexEvents = TRUE, | ||
| runTemporalCohortCharacterization = TRUE, | ||
| runIncidenceRate = TRUE, | ||
| runIncludedSourceConcepts = TRUE, | ||
| runOrphanConcepts = TRUE, | ||
| runTimeSeries = TRUE, | ||
| runCohortRelationship = TRUE, | ||
| minCellCount = 0, | ||
| incremental = TRUE, | ||
| incrementalFolder = incrementalFolder, | ||
| temporalCovariateSettings = server$temporalCovariateSettings, | ||
| runOnSample = FALSE | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This test introduces debugonce(executeDiagnostics), which will break non-interactive/CI test runs, and the file later references variables like folder, dbms, and connectionDetails that are not defined in this scope. Remove debugonce() and ensure all referenced variables come from server (or are initialized in the test) so the test is runnable.
| fileLoc <- system.file( | ||
| 'images', | ||
| "logo.png", | ||
| package = "OhdsiShinyModules" |
There was a problem hiding this comment.
This helper looks up assets in the OhdsiShinyModules package, but this PR removes OhdsiShinyModules as a dependency. Switch system.file(..., package = 'CohortDiagnostics') (and ensure the image is shipped under this package’s inst/), otherwise the app will return an empty path at runtime.
Suggested change
| package = "OhdsiShinyModules" | |
| package = "CohortDiagnostics" |
Comment on lines
+1
to
+8
| getPredictionHelp <- function(file){ | ||
| fileLoc <- system.file( | ||
| 'patient-level-prediction-www', | ||
| file, | ||
| package = "OhdsiShinyModules" | ||
| ) | ||
| return(fileLoc) | ||
| } |
There was a problem hiding this comment.
This helper appears copied from PatientLevelPrediction/OhdsiShinyModules (path patient-level-prediction-www and package lookup). If it is intended for CohortDiagnostics, rename it and point it to CohortDiagnostics-owned help assets; otherwise remove it to avoid confusing/unused Shiny module utilities.
Suggested change
| getPredictionHelp <- function(file){ | |
| fileLoc <- system.file( | |
| 'patient-level-prediction-www', | |
| file, | |
| package = "OhdsiShinyModules" | |
| ) | |
| return(fileLoc) | |
| } |
| #' @param id Namespace Id - use namespaced id ns("orphanConcepts") inside diagnosticsExplorer module | ||
| #' | ||
| #' @export | ||
| orpahanConceptsView <- function(id) { |
There was a problem hiding this comment.
The exported UI function name has a typo (orpahanConceptsView). Consider renaming to orphanConceptsView for a stable public API and updating all internal call sites accordingly (keeping a deprecated alias if needed for backward compatibility).
| style = "text-align:right;", | ||
| withTooltip(shiny::tags$button(buttonText, | ||
| onclick = paste0("Reactable.downloadDataCSV('", ns(outputTableId), "')")), | ||
| tooltip = "Note, will not download live values filtered in table, groupings, or any graphical/stylstic elements") |
There was a problem hiding this comment.
Correct typo in tooltip text: 'stylstic' → 'stylistic'.
Suggested change
| tooltip = "Note, will not download live values filtered in table, groupings, or any graphical/stylstic elements") | |
| tooltip = "Note, will not download live values filtered in table, groupings, or any graphical/stylistic elements") |
Comment on lines
+304
to
320
| data <- data %>% dplyr::select("cohortCount", "personYears", "gender", "ageGroup", | ||
| "calendarYear", "incidenceRate", "cohortId", "databaseId") | ||
| exportDataToCsv( | ||
| data = data, | ||
| tableName = "incidence_rate", | ||
| fileName = file.path(exportFolder, "incidence_rate.csv"), | ||
| minCellCount = minCellCount, | ||
| databaseId = databaseId, | ||
| incremental = incremental, | ||
| # incidenceRate is 1000 * count / personYears; censor when implied count < minCellCount | ||
| enforceMinCellValueFunc = function(data) { | ||
| minRateThreshold <- 1000 * minCellCount / as.numeric(data$personYears) | ||
| minRateThreshold[!is.finite(minRateThreshold)] <- Inf | ||
| enforceMinCellValue(data, "incidenceRate", minRateThreshold) | ||
| }, | ||
| cohortId = subset$cohortId | ||
| ) |
There was a problem hiding this comment.
runIncidenceRate() uses incremental cohort selection but does not record completion (no recordTasksDone(...) call like other runners). In incremental mode this will cause the same cohorts to be recomputed on subsequent runs. Add a recordTasksDone() call after a successful export (and ensure subset$checksum is available).
Comment on lines
+71
to
+75
| "FROM @cdmDatabaseSchema.VOCABULARY", | ||
| "WHERE vocabulary_id = 'None'") | ||
| dbVersion <- DatabaseConnector::renderTranslateQuerySql(connection = connection, | ||
| sql = sql, | ||
| cdmDatabaseSchema = cdmDatabaseSchema) |> |
There was a problem hiding this comment.
The SQL uses a placeholder name @cdmDatabaseSchema and an uppercase table VOCABULARY. This is likely to break on engines with case-sensitive identifiers and is inconsistent with other package SQL that uses @cdm_database_schema.vocabulary. Use consistent SqlRender parameter naming (e.g. @cdm_database_schema) and reference vocabulary in the expected casing to avoid cross-DB failures.
Suggested change
| "FROM @cdmDatabaseSchema.VOCABULARY", | |
| "WHERE vocabulary_id = 'None'") | |
| dbVersion <- DatabaseConnector::renderTranslateQuerySql(connection = connection, | |
| sql = sql, | |
| cdmDatabaseSchema = cdmDatabaseSchema) |> | |
| "FROM @cdm_database_schema.vocabulary", | |
| "WHERE vocabulary_id = 'None'") | |
| dbVersion <- DatabaseConnector::renderTranslateQuerySql(connection = connection, | |
| sql = sql, | |
| cdm_database_schema = cdmDatabaseSchema) |> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work in progress.