Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impute zeroes variable collections #26

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

d-callan
Copy link
Contributor

oops. forgot about collections when i built the impute zeroes helper..

@d-callan d-callan requested review from bobular and asizemore August 28, 2023 17:30
Copy link
Member

@asizemore asizemore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of questions - maybe the three of us could meet and go through it? I'm having a hard time keeping it all straight!

R/methods-Megastudy.R Outdated Show resolved Hide resolved
R/methods-Megastudy.R Show resolved Hide resolved
@@ -122,16 +142,54 @@ findEntityIdColumnNameForVariableSpec <- function(varSpec, entityIdColumns) {
return(entityIdColumns[grepl(varSpec@entityId, entityIdColumns)])
}

findStudyVocabularyByVariableSpec <- function(vocabs, variableSpec) {
findStudyVocabularyByVariableSpec <- function(vocabs, variables, variableSpec) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this and the previous function should be s4 methods since we're going through the trouble of checking their classes? Not urgent but for the future

R/methods-Megastudy.R Outdated Show resolved Hide resolved
R/methods-Megastudy.R Show resolved Hide resolved
return(varSpecsWithVocabs)
}

getVariableColumnNames <- function(variableMetadata) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s4 method?

R/methods-VariableMetadata.R Show resolved Hide resolved
tests/testthat/test-class-Megastudy.R Show resolved Hide resolved
tests/testthat/test-class-Megastudy.R Show resolved Hide resolved
hasStudyDependentVocabulary = TRUE,
members = VariableSpecList(S4Vectors::SimpleList(VariableSpec(variableId='pathogen_presence', entityId='assay'),
VariableSpec(variableId='pathogen2_presence', entityId='assay'),
VariableSpec(variableId='pathogen3_presence', entityId='assay')))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i need a test where i have a collection, but the OTHER var is the one w the special vocab

@d-callan d-callan merged commit 3531e79 into main Sep 7, 2023
@d-callan d-callan deleted the impute-zeroes-variable-collections branch September 7, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants