-
Notifications
You must be signed in to change notification settings - Fork 10
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
82 adlbc derivations #92
Conversation
Hi, @nicolekjones . Many thanks for this update and appreciate your flexibility to check on your code and adapt to the updated R environment. One thing I noticed in this PR is that there is a conflict with the specs being used. We did merge a new set of specs from #90 which should be in main now. I just wanted to make sure you did this :
Before you made the update to the ADLBC specs above? |
I thin i missed that last step. I did: git checkout main I believe there was a merge that happened between creating the branch and my final push I’ll resolve this afternoon 😁 |
Thanks, @nicolekjones . I see now the bumps we can run into with the git workflow. I'm still learning myself. :) |
Merge branch 'main' into 82-adlbc-derivations # Conflicts: # adam/TDF_ADaM - Pilot 3 Team updated.xlsx
@laxamanaj Looks like I resolved merge conflict! Checks are running. If any have issue I'll resolve after they finish. I think merge conflicts are often part of the journey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @nicolekjones . Thanks for the PR. I've added some comments for lines 198 and 244. Best,
Joel
@laxamanaj, just pushed the updates. For those full_joins yes we do expect a one to many merge. updated so the warning is suppressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @nicolekjones . I have one more comment regarding lines 36-40.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @nicolekjones . Thanks for all of your help on making these updates. Apologes, one last thing, I believe we should update. After reviewing the code, I see lines 36-40 seem to depend on the CDISCPlot01 adam/ADLBC. Can we remove this if not needed as to not confuse FDA why we are reading another version of ADLBC?
## Analysis Dataset Lab Blood Chemistry
prodc <- convert_blanks_to_na(read_xpt(file.path("adam", "adlbc.xpt")))
# Variables for programming
toprogram <- setdiff(colnames(prodc), c(colnames(lb), unique(supplb[["QNAM"]])))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I think we put this in initially to see what was needed, but you're right we don't need it.
After running metacore I am getting these warnings
Metadata successfully imported
Warning messages:
1: core from the ds_vars table only contain missing values.
supp_flag from the ds_vars table only contain missing values.
dataset from the supp table only contain missing values.
variable from the supp table only contain missing values.
idvar from the supp table only contain missing values.
qeval from the supp table only contain missing values.
2: The following variables are in the ds_vars table, but don't have value specs:
ADLBC.AVAL, ADTTE.AVAL
3: The following derivations are never used:
ADLBC.ABLFL: Y if VISITNUM=1, null otherwise
ADLBC.AGE: ADSL.AGE
ADLBC.AGEGR1: ADSL.AGEGR1
ADLBC.AGEGR1N: ADSL.AGEGR1N
ADLBC.AVAL: LB.LBSTRESN
ADLBC.COMP24FL: ADSL.COMP24FL
ADLBC.DSRAEFL: ADSL.DSRAEFL
ADLBC.LBNRIND: LB.LBNRIND
ADLBC.LBSEQ: LB.LBSEQ
ADLBC.LBSTRESN: LB.LBSTRESN
ADLBC.PARAMCD: LBTESTCD
ADLBC.RACE: ADSL.RACE
ADLBC.RACEN: ADSL.RACEN
ADLBC.SAFFL: ADSL.SAFFL
ADLBC.SEX: ADSL.SEX
ADLBC.STUDYID: ADSL.STUDYID
ADLBC.SUBJID: ADSL.SUBJID
ADTTE.AVAL: ADT-STARTDT+1
4: The following codelist(s) are never used:
Medical Dictionary for Regulatory Activities
I'm not 100% sure how to resolve these or if it's a concern. I'm also not sure why ADTTE.AVAL is showing up here .
It appears to be from variables that all are predecessors. I noticed that other datasets provided a derivation in the Methods for variables like AGE despite them being predecessors, so I followed that practice. Do you think I should remove predecessors from Methods tab?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for notifying me of the metacore spec issues. This can be fixed.
Essentially the way the specs work, only should a variable be included in the Methods tab, if in the Variables tab Origin='Derived'. Otherwise, if Origin='Predecessor', then the variable should not be in the Methods tab.
So for example, for ADLBC.AGE, this warning is saying that this derivation has never been used, because in the Variables tab for ADLBC.AGE, it is set to Predecessor, but if it was set to Derived it will be ok to keep in the Methods tab. So there are 2 ways to fix it.
-
In the Variables tab, for this example, you can set ADLBC.AGE to Origin='Derived' & Method='ADLBC.AGE' (this will be the corresponding ID in the Methods tab). Then in the Methods tab, leave the row ADLBC.AGE as-is.
-
In the Variables tab, for this example, you can leave ADLBC.AGE set to Origin='Predecessor', then in the Methods tab , delete/remove the row ADLBC.AGE, since it is not identified as Origin='Derived'. This approach answers your original question.
Similar approach should be taken for the rest of the variables in this warning, this will then fix bullet 3.
I hope this makes sense. Let me know if you want discuss in meeting also. Happy to do so.
Finally, once the warning in bullet 3 are resolved, then the other warnings can be left as-is and you can add the argument quiet=T
, which will then mute these warnings in the run. For example : metacore <- spec_to_metacore("adam/TDF_ADaM - Pilot 3 Team updated.xlsx", where_sep_sheet = FALSE, quiet = T)
As for why ADTTE.AVAL is showing up here, I'm not sure as the Variables tab and Methods tab seem be set accordingly. @bms63 or @DeclanHodges , wondering if you may have some insight on this on the GSK side of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense! I was under the impression that predecessors shouldn't have entries in the methods tab, but I thought maybe I was wrong. Removed them and added the quiet = true.
I was wondering the same thing with adtte.aval. I looks fine and when I check, there's no link between ADTTE and ADLBC.
Let me know if I need to make any further changes!
Many thanks for these swift updates, @nicolekjones . I will merge now. |
Hi all,
I did a bit of cleaning of the code
Specs were updated