OHDSI / CohortDiagnostics

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

error when generateInclusionStats = FALSE but SQL has inclusionStats #35

Closed gowthamrao closed 4 years ago

gowthamrao commented 4 years ago

This issue is when we are instantiating and generating cohort statistics based on JSON/SQL specifications in package - created using ROhdsiWebApi::insertCohortDefinitionInPackage function.

If the specifications were created using ROhdsiWebApi::insertCohortDefinitionInPackage1 function withgenerateStats = TRUE`, sql will have snippets on inclusion stats. These snippets use 'results_database_schema', not 'target_database_schema'.

If these specifications are called by function generateInclusionStats of CohortDiagnostics with generateInclusionStats = FALSE - it will throw an error because SqlRender is missing results_database_schema.

Error: java.sql.SQLException: Invalid operation: syntax error at or near "@" Position: 13;

SQL: delete from @results_database_schema.cohort_inclusion_result where cohort_definition_id = 15234 and mode_id = 0

Error from Function `instantiateCohortSet' https://github.com/OHDSI/CohortDiagnostics/blob/29aaaf756e86f1a285461c7cba220cb2f5894675/R/CohortConstruction.R#L429

https://github.com/OHDSI/CohortDiagnostics/blob/29aaaf756e86f1a285461c7cba220cb2f5894675/R/CohortConstruction.R#L523

schuemie commented 4 years ago

Yes, this is unfortunately unavoidable: if you imported the cohort definitions with the inclusion rule statistics logic embedded in the SQL, you must run the inclusion stats when instantiating them.

It would be good to mark this clearly in the documentation, and throw a meaningful error when the user tries to do it differently anyway. The best way to detect whether the SQL includes the inclusion rule stats is probably to do a regex search for one of the statistics tables that is required. If the table is there, but the user has inclusion statistics turned of, we should throw a clear error.

@gowthamrao : would like to give that a try?

gowthamrao commented 4 years ago

yes. i will assign this to myself, but will combine this as part of a broader release.

gowthamrao commented 4 years ago

if runInclusionStatistics = TRUE, we either need a default (not NULL) value for inclusionStatisticsFolder or we need user to provide it.