Use external concept counts#1117
Conversation
azimov
left a comment
azimov
left a comment
There was a problem hiding this comment.
Overall I think this could speed up the phenotyping process quite significantly - using Achilles counts as well somewhat conflicts with this branch. I think that is probably a better approach in general as users should be running this anyway as part of an ETL process.
However, I don't want to hold things up too much. This code should make it into the next release with a few minor changes suggested here.
|
Hi! I tried to cover most of the comments, please let me know if there is any more feedback. Especially for the testing. Thanks a lot, and sorry for the delay. |
| #' diagnostics to. | ||
| #' @param cohortDefinitionSet Data.frame of cohorts must include columns cohortId, cohortName, json, sql | ||
| #' @param cohortTableNames Cohort Table names used by CohortGenerator package | ||
| #' @param conceptCountsTable Concepts count table name. The default is "#concept_counts" to create a temporal concept counts table. |
There was a problem hiding this comment.
THese changes look good but you need to run devtools::document() (or the build in rstudio) to add the changes to include these parameters. Doing this should get the build to finish and I can then merge in your changes.
azimov
left a comment
azimov
left a comment
There was a problem hiding this comment.
Two minor changes:
- The tests should be improved to run on all platforms, not just sqlite
- The build fails because the new arguments are not documented yet with devtools::document
I can likely fix the latter one once merged in to develop. However, I would like tests to at least pass before doing this
|
|
||
| # Creating externalConceptCounts | ||
| sql_lite_path <- file.path(test_path(), "4448testEunomia.sqlite") | ||
| connectionDetails <- createConnectionDetails(dbms= "sqlite", server = sql_lite_path) |
There was a problem hiding this comment.
Please don't stope a test sqlite file - instead use Eunomia::getEunomiaConnectionDetails().
If to use a dataset that is not the standard MI data you can download it prior to running the tests:
https://ohdsi.github.io/Eunomia/reference/downloadEunomiaData.html
| test_that("Creating and checking externalConceptCounts table", { | ||
|
|
||
| # Creating externalConceptCounts | ||
| sql_lite_path <- file.path(test_path(), "4448testEunomia.sqlite") |
There was a problem hiding this comment.
This file was not checked in, so tests fail. However, I would not like to add this file unless you have a specific use case for it (e.g. some specifically modified tables - but then I would prefer to see the sql that generates this as it will be easier to maintain).
| conceptCountsTableIsTemp = FALSE, | ||
| removeCurrentTable = TRUE) | ||
|
|
||
| concept_counts_info <- querySql(connection, "PRAGMA table_info(concept_counts)") |
There was a problem hiding this comment.
This command will only work on sqlite
| expect_equal(result$a, c(1, 2, 3, 4, 5)) | ||
| }) | ||
|
|
||
| test_that("Creating and checking externalConceptCounts table", { |
There was a problem hiding this comment.
This test will only run on sqlite. I would prefer tests across all database platforms. These are setup in the setup.R file for you:
https://github.com/OHDSI/CohortDiagnostics/blob/main/tests/testthat/setup.R
So you can just use the connectionDetails variable which should already be available to you.
For example see this test:
…s not already exist. Skip results datamode test if SKIP_DB_TESTS is TRUE.
|
What is the current status of this? is it added to an upcoming release? and if so when is this release planned (there are no due dates on the milestones in this package)? |
Current status is that it will be included in the upcoming release. There are a few things left to do to get R check working. It is part of the 3.4 release - https://github.com/OHDSI/CohortDiagnostics/milestone/3 There is no planned release date yet. @azimov should we set a release date for 3.4? |
The other changes that are in the 3.4.0 are relatively small and shouldn't take long - we can set a date for a few weeks from now or whenever this PR is finished |
|
@ablack3 @cebarboza Any update on this PR? I would like to start testing 3.4.0 next week. If this can make it in it would be great, otherwise we can shelve this for a future date. |
|
Hi @azimov, I'm looking at it now. Your request that we test the createConceptCounts table on all database systems will make the tests run longer. I made the change though so we can see how long it takes. |
Merge branch 'useExternalConceptCounts' of github.com:ohdsi/CohortDiagnostics into useExternalConceptCounts # Conflicts: # tests/testthat/test-other.R
4 participants
Hi,
I would like to submit this contribution related to #1067.
externalConceptCounts()to create that table in the write schema.useExternalConceptCountsTableparameter in runDiagnostics.useExternalConceptCountsTablewhen TRUE or create a new table as default when FALSE.Please let me know what you think to see if we can work on this changes!