Big int cohort id fix #163
Conversation
This reverts commit de27d57.
# Conflicts: # DESCRIPTION
R/Analyses.R
Outdated
| checkmate::assertLogical(outcomeOfInterest, add = errorMessages) | ||
| checkmate::assertNumeric(trueEffectSize, len = 1, null.ok = TRUE, add = errorMessages) | ||
| checkmate::assertInt(riskWindowStart, null.ok = TRUE, add = errorMessages) | ||
| checkmate::assertInt(riskWindowEnd, null.ok = TRUE, add = errorMessages) | ||
| checkmate::reportAssertions(collection = errorMessages) | ||
| if (!is.null(startAnchor) && !grepl("start$|end$", startAnchor, ignore.case = TRUE)) { | ||
| stop("startAnchor should have value 'cohort start' or 'cohort end'") | ||
| stop("startAnchor should have value \'cohort start\' or \'cohort end\'") |
There was a problem hiding this comment.
Could you help me understand why you're escaping single quotes? It is not needed when the string uses double quotes
R/RunAnalyses.R
Outdated
| createCmDataTask <- function(i) { | ||
| refRow <- subset[i, ] | ||
| refRow <- subset[i,] |
There was a problem hiding this comment.
Per our code guidelines, a comma should be followed by a space. Why remove it here?
R/RunAnalyses.R
Outdated
| ) | ||
| return(task) | ||
|
|
||
| if (file.exists(file.path(outputFolder, refRow$psFile))) { |
There was a problem hiding this comment.
Why the condition? Is there something wrong with the code for generating the reference table that will specify strataFiles to be created without there being psFiles? Maybe best to fix it there?
R/RunAnalyses.R
Outdated
| studyPop <- readRDS(file.path(outputFolder, refRow$studyPopFile)) | ||
| studyPop <- addPsToStudyPopulation(studyPop, ps) | ||
| saveRDS(studyPop, file.path(outputFolder, refRow$psFile)) | ||
| tryCatch({ |
There was a problem hiding this comment.
Is the error message thrown by saveRDS not informative enough? If so, maybe we should have a generic wrapper for saveRDS in CohortMethod that we always use?
R/RunAnalyses.R
Outdated
| params$psFile)) | ||
| ps <- applyTrimMatchStratify(ps, params$args) | ||
| saveRDS(ps, params$strataFile) | ||
| tryCatch({ |
There was a problem hiding this comment.
Yikes, this tryCatch block covers multiple function calls, but the error message will be about the saving of the stataFile.
R/RunAnalyses.R
Outdated
| if (length(covariatesToExclude) != 0) { | ||
| covariates <- covariates %>% | ||
| filter(!.data$covariateId %in% covariatesToExclude) | ||
| tryCatch({ |
There was a problem hiding this comment.
Double yikes! What is the point in wrapping such a large block of code in a try catch?
R/RunAnalyses.R
Outdated
| attr(class(filteredCohortMethodData), "package") <- "CohortMethod" | ||
| saveCohortMethodData(filteredCohortMethodData, params$prefilteredCovariatesFile) | ||
| }, error = function(err) { | ||
| ParallelLogger::logError(err) |
There was a problem hiding this comment.
This code has no effect. Errors are automatically caught by ParalllelLogger. Just rethrowing the error will achieve the same thing
|
I'm sorry, I can't accept this PR. It contains a lot of changes that have nothing to do with the issue at hand. I'm open to rethinking how we deal with errors, but that should be a separate issue and a separate PR (but only after we agree on the approach). Also, please adhere to the style guide (as also implemented by the styler package). |
|
My apologies, I did a diff with the wrong branch and included the code I used to find issues. This now has been removed.
From: Martijn Schuemie ***@***.***>
Date: Sunday, April 7, 2024 at 10:20 PM
To: OHDSI/CohortMethod ***@***.***>
Cc: Gilbert, James [JANUS] ***@***.***>, Author ***@***.***>
Subject: [EXTERNAL] Re: [OHDSI/CohortMethod] Big int cohort id fix (PR #163)
I'm sorry, I can't accept this PR. It contains a lot of changes that have nothing to do with the issue at hand.
I'm open to rethinking how we deal with errors, but that should be a separate issue and a separate PR (but only after we agree on the approach).
Also, please adhere to the style guide (as also implemented by the styler package).
—
Reply to this email directly, view it on GitHub<#163 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABMDM4GAOJGV4FUXTWTQ53Y4ISKNAVCNFSM6AAAAABFV6W7U2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRHA4DINZWGY>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Thanks! Not sure why we're failing check now. Seems to be some SQL issue. I'll merge and debug |
|
Ah, the problem is that SQLite does not allow changing the column type of an existing table. The good news is that SQLite INT is dynamic, and will contain BIGINT values. Why do we have cohort IDs > 2^31? |
These are generally cohorts made from templates or subsets where we do something like |
|
Ok, we'll probably need fixes throughout HADES then. I always assumed, since we're assigning cohort IDs ourselves, we would not exceed 2^31. I think I fixed the unit tests now, by fixing the SqlRender translation of I also removed the PostgreSQL-specific version of the migration script. We really shouldn't have platform-specific code in HADES packages other than SqlRender and DatabaseConnector. |
Resolves #162