NotApplicable fix for measureConditionEraCompleteness #527
Conversation
|
I see the checks ar failing, I will investigate and fix this. |
|
Thanks @MaximMoinat ! Love the initiative. I'll wait to do a deep review til you've sorted out the failing checks. I'd also recommend to add some unit tests for the code. I know there weren't any test specific to NA before, but now that they're in their own function and we're refactoring a bit, it feels like the right time. Might need to get a bit crafty to test this though :) off the top of my head, I'm picturing truncating a table in Eunomia and then seeing if related checks are marked NA. Happy to help brainstorm further on that. |
|
I like the idea of adding tests, will look into that.
That is exactly the test I did locally already: run with truncated condition occurrence (expecting NA) and with truncated condition era (expecting fail). This passed for me, but would indeed be good to have this integrated into the test suite. |
|
@katy-sadowski Tests are fixed and new tests added. Note that these are in a separate file "test-calculateNotApplicableStatus.R", although they do not test the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #527 +/- ##
===========================================
+ Coverage 86.48% 88.46% +1.98%
===========================================
Files 16 17 +1
Lines 888 954 +66
===========================================
+ Hits 768 844 +76
+ Misses 120 110 -10 ☔ View full report in Codecov by Sentry. |
| on.exit(unlink(outputFolder, recursive = TRUE)) | ||
|
|
||
| # Remove records from Condition Occurrence | ||
| connection <- DatabaseConnector::connect(connectionDetailsEunomia) |
There was a problem hiding this comment.
i think it would be safer to create a separate eunomia DB just for these tests. i believe testthat runs the test files in parallel so we might run into weird issues if we're modifying the same DB here.
There was a problem hiding this comment.
I don't think this is an issue. If I open a Eunomia connection in two R sessions, clearing a table in one session does not affect the other session.
There was a problem hiding this comment.
I believe tests are all being run in the same R session though. And I checked:
> cd <- Eunomia::getEunomiaConnectionDetails()
attempting to download GiBleed
trying URL 'https://raw.githubusercontent.com/OHDSI/EunomiaDatasets/main/datasets/GiBleed/GiBleed_5.3.zip'
Content type 'application/zip' length 6861852 bytes (6.5 MB)
==================================================
downloaded 6.5 MB
attempting to extract and load: /var/folders/l2/3d9d5v9s197_2nkdvgrljv4c0000gn/T//RtmpM53RnU/GiBleed_5.3.zip to: /var/folders/l2/3d9d5v9s197_2nkdvgrljv4c0000gn/T//RtmpM53RnU/GiBleed_5.3.sqlite
> con <- DatabaseConnector::connect(cd)
Connecting using SQLite driver
> p <- DatabaseConnector::querySql(con, "select * from person")
> nrow(p)
[1] 2694
> DatabaseConnector::executeSql(con, "delete from person")
|=======================================================================| 100%
Executing SQL took 0.0128 secs
> nrow(p)
[1] 0
> con2 <- DatabaseConnector::connect(cd)
Connecting using SQLite driver
> p2 <- DatabaseConnector::querySql(con, "select * from person")
> nrow(p2)
[1] 0
There was a problem hiding this comment.
@MaximMoinat I think this got lost in the shuffle - let me know what you think and if you're OK creating new connection details for these tests. Thanks!
There was a problem hiding this comment.
You are right. I have now added a separate NaChecks cd object for these tests. This seems to work as expected:
> connectionDetailsEunomiaNaChecks <- Eunomia::getEunomiaConnectionDetails()
...
attempting to extract and load: /Users/maxim/eunomia_data/GiBleed_5.3.zip to: /Users/maxim/eunomia_data/GiBleed_5.3.sqlite
> connection <- DatabaseConnector::connect(connectionDetailsEunomia)
Connecting using SQLite driver
> DatabaseConnector::executeSql(connection, "DELETE FROM CONDITION_OCCURRENCE;")
> DatabaseConnector::querySql(connection, "select count(*) from condition_occurrence;")
COUNT(*)
1 0
> DatabaseConnector::disconnect(connection)
> connectionDetailsEunomia <- Eunomia::getEunomiaConnectionDetails()
...
attempting to extract and load: /Users/maxim/eunomia_data/GiBleed_5.3.zip to: /Users/maxim/eunomia_data/GiBleed_5.3.sqlite
> connection <- DatabaseConnector::connect(connectionDetailsEunomia)
Connecting using SQLite driver
> DatabaseConnector::querySql(connection, "select count(*) from condition_occurrence;")
COUNT(*)
1 65332
> DatabaseConnector::disconnect(connection)
> connection <- DatabaseConnector::connect(connectionDetailsEunomiaNaChecks)
Connecting using SQLite driver
> DatabaseConnector::querySql(connection, "select count(*) from condition_occurrence;")
COUNT(*)
1 0
> DatabaseConnector::disconnect(connection)
|
Thanks for your comments Katy. I agree with your suggestions and will implement these. |
|
@katy-sadowski Implemented the requested changes, except for the one on the test. See my comment. |
Fixes #526, by having an exception for measureConditionEraCompleteness check; it will be NA when condition occurrence table is missing or empty.
To be able to make the fix, I had to refactor the NA status code. While doing that, I also moved the logic to a separate function.
As a reminder when/if we add measureDrugEraCompleteness, I have added some code for that already but commented it out for now.
TODO:
.calculateNAstatus