Trim demographics #94
Conversation
|
hi @nmercadeb @edward-burn I think this needs more testing, will need to add it during the week :) |
R/trimDemographics.R
Outdated
| sex = NULL, | ||
| minPriorObservation = NULL, | ||
| minFutureObservation = NULL, | ||
| order = c("sex", "age", "prior_observation", "future_observation"), |
There was a problem hiding this comment.
would be good to discuss this pattern (on the face of it I don't really like it as it doesn't quite fit the rest of the package, but would need to understand better why it is here)
R/observationPeriodCohort.R
Outdated
| name <- validateName(name) | ||
| cohortName <- assertCharacter(cohortName, length = 1) | ||
|
|
||
| cdm[[name]] <- cdm$observation_period |> |
There was a problem hiding this comment.
It would be good I think to inner_join with the person table. Normally people in person would also be in observation period (and vice versa) but i have seen cases where this is not true
R/observationPeriodCohort.R
Outdated
| # TO BE EXPORTED WHEN TESTS ARE ADDED | ||
| #' @noRd | ||
| #' | ||
| observationPeriodCohort <- function(cdm, |
There was a problem hiding this comment.
We could make the name a little more general to indicate this is person + observation period tables (see comment below)
In a way I would be tempted to broaden and make a demographicCohort() function .....
catalamarti
requested review from
edward-burn and
nmercadeb
and removed request for
edward-burn
|
this should be ready for a second review @edward-burn @nmercadeb |
No description provided.