Cohort subset to allow custom windowing #197
Conversation
|
|
||
| # Start and end windows must always have subset anchor values set to support backwards compatibility | ||
| if (!is.null(startWindow)){ |
There was a problem hiding this comment.
THis section forces the old logic upon the user, which I dislike, but it also preseves the exact logic of the previous implementation. To me there is no perfect solution here...
| cohortWindowLogic <- lapply(private$operator$windows, function(window) { | ||
| lsql <- " AND (S.@s_cohort_anchor >= DATEADD(d, @window_start_day, T.@window_anchor) AND S.@s_cohort_anchor <= DATEADD(d, @window_end_day, T.@window_anchor))" | ||
| SqlRender::render(lsql, | ||
| window_anchor = ifelse(window$targetAnchor == "cohortStart", |
There was a problem hiding this comment.
If you're inclined, in the CI package the choice of start/end is simply 'start' or 'end' seen here:https://github.com/OHDSI/CohortIncidence/blob/09cf6aa3a6f4cfdbe99d04e38b61a85441fa3925/R/Classes.R#L547. If we want to adopt that moniker for choosing a start or end date to those choices instead of 'cohortStart' or 'cohortEnd'. I origionally had CI using same thing (with cohort in the label) but switched it out to be not cohort-specific but just simply a choice of start/end. Just pointing out where we might be consistent across packages.
There was a problem hiding this comment.
Great work @azimov - I made a small change in 46d5903 to guide users towards using the newly implemented windows parameter and that we'd deprecate the startWindow and endWindow parameters. Let me know if you have any objections.
I'm also going to open a new issue that will focus on the documentation of cohort subsetting, inclusive of these changes, to provide more working examples and details on how to use the various subset operators in the package.
This is designed to resolve Issue #175