Conditionally use timeId in computeStandardizedDifference in temporal covariate data #226
Conversation
gowthamrao
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #226 +/- ##
===========================================
- Coverage 93.34% 91.44% -1.91%
===========================================
Files 16 16
Lines 1412 1449 +37
===========================================
+ Hits 1318 1325 +7
- Misses 94 124 +30 ☔ View full report in Codecov by Sentry. |
| @@ -59,31 +59,52 @@ computeStandardizedDifference <- function(covariateData1, covariateData2, cohort | |||
| if (!isAggregatedCovariateData(covariateData2)) { | |||
| stop("Covariate2 data is not aggregated") | |||
| } | |||
| if (!setequal(colnames(covariateData1$covariates), colnames(covariateData1$covariates))) { | |||
There was a problem hiding this comment.
you are now comparing covariateData1 to covariateData1, one of these should be covariateData2. Also, could you put this check after the null check of the covariates?
There was a problem hiding this comment.
@ginberg , i suspect there is something logically incorrect in this code (not my PR, but the original released code). e.g. i am getting results when sometimes mean1 > mean2 has 0 records, or mean2 > mean1 has 0 records. Also, i find it hard to read the code. I will do another PR for your consideration that is a refactor of this full script
There was a problem hiding this comment.
@gowthamrao ok, that's good to know. Thanks for your refactoring. It would be helpful if you could provide a reprex of a situation in which you are getting logically incorrect results, could you add that?
There was a problem hiding this comment.
@ginberg I have done the PR with the refactor. I don't know how to do the reprex and it will probably take too much time to emulate the complex feature extraction output.
|
@gowthamrao can we close this PR, since you opened this new one? #228 |
|
@gowthamrao closing in favor of #228. |
#225
Seems to fix the issue