-
Notifications
You must be signed in to change notification settings - Fork 11
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
FDA Feedback: Closes #152 Derive LOCF only considering ANL01FL=Y #146
Conversation
Hi @kaz462, the lintr is failing due to commented out code, mutate( ) function etc. Please let me know if you want me to correct. thx. |
@robertdevine that would be great, thanks! I'm not sure if this PR will be merged though, let's check with FDA this Thursday? Thank you, @kaz462, will do on the lintr check item, and best as you noted, to confirm FDA Reviewers feedback this Thursday before decision to merge the PR. thx. All set @kaz462, lintr check was failing due to (i)commented out code at mutate( ) and (ii)superfluous white spaces at adas_locf2 (line 129) and 'order = exprs(AVISITN, AVISIT),' (line 139). thx. |
Hi, @kaz462 . Thanks again for working on this and showing that ANL01FL ='Y' is what matches the original Pilot 1 results. We discussed this in a smaller meeting, but just noting here that we'll touch base with FDA on our findings to then see which approach would be ok to move forward. In the end, we may just want to match with Pilot 1 though may not be the approach to take, but we want to show that our R submission works including ADaMs. I'll confirm a merge after meeting with FDA. |
qc findings are updated here: https://github.com/RConsortium/submissions-pilot3-adam/wiki/QC-Findings#adadas |
@laxamanaj, @kaz462, @bms63 - should the SAP be updated with a statement about the visit window rule used to tidy the output and match the original pilot1? - it appears to follow from the earlier observation regarding analysis visits. The visit window rule approach specified in the SAP is translated into the programming specifications for Analysis Data Model (ADaM). Visit window rules sometimes appear as a separate tab in the ADaM specs Excel spreadsheet, or as a part of the specs for each corresponding ADaM dataset. thx. |
submission/programs/adadas.r
Outdated
filter = !is.na(AVISIT) | ||
) | ||
|
||
################# Test: derive LOCF only considering ANL01FL=Y |
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.
Could we do a comment on why we this derivation is the way it is - feedback from FDA on need to match Pilot 1 - and maybe even provide github wiki qc finding link?
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.
@bms63 good point! updated
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.
I updated the PR title a bit. Just a comment to insert in the code is suggested.
I think I changed it to Squash and Merge so the PR title is the commit message. Feel free to merge in - just make sure it is Squash and Merge
@robertdevine I don't think we should update the SAP - we should just put it in the findings/feedback notes @bms63 - maybe we'll add a 'Determination' paragraph onto the findings/feedback notes here under Issue#2 following the Action Item, and Observation entries. Doing so may provide further detail about the issue determination for visitors observing the pilot series and the regulatory feedback process. 21 CFR Part 11 comes to mind regarding clinical data - reliable, consistent, and accurate for regulatory review trace purposes. thx. |
…ns-pilot3-adam into adadas_locf_update # Conflicts: # submission/programs/adadas.r
@laxamanaj, @bms63, @kaz462 - Pilot3 Team should be aware that pilot1wrappers remains in the pilot3 reproducible environment. A very simple test regarding the Primary Endpoint Analysis: ADAS Cog (11) - Change from Baseline to Week 24 - LOCF might be to use the pilot3 repository and pilot1wrappers separately from pilot3 (and its pilot3utils dependency) to see if the discrepancies pointed out by FDA Reviewers can be reproduced (contained within) the pilot3 repository (given the existing inclusion of pilot1wrappers). There are only six steps in the primary program and a simple test may tease out if pilot1wrappers inclusion in pilot3 repository can reproduce the pilot1 primary outputs (using pilot1wrappers only) running from the pilot3 repository. If so, removing pilot1wrappers (keeping everything else the same in the pilot3 repository) with the exception of changing primary output library dependency to pilot3 and its dependency on pilot3utils (then removing pilot1wrappers from the pilot3 reproducible environment) to check if the primary outputs match. Has anyone on Pilot3 Team performed this test? Seems reasonable and compelling to do so given the FDA feedback regarding primary output discrepancies between pilot1 and pilot3 and this outstanding issue about pilot1wrappers and refactoring the lock file here. thx. |
Hi Robert - pilot1 versus pilot3I thought all the pilot1 wrapper programs were put into the @bms63, @kaz462, @laxamanaj - searching some of the CDISC pilot replication sources - for example here, FDA Reviewers Observations - pilot1 and pilot3 (highlighted) CDISC Replication Effort (matches Pilot 1 primary output) - compare with highlighted values in pilot3 output fda findingsMy impression of the discussion was how we selected the observations in pilot3 was the primary driver in the differences in pilot1 versus pilot3. This update form @kaz462 should bring us into alignment with pilot1. As far I knew, the code in the pilot3utils package is identical to the pilot1 helper code. Thank you, @bms63 |
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.
Minor comment regarding comments. Otherwise, agree with the updates here.
submission/programs/adadas.r
Outdated
filter = !is.na(AVISIT) | ||
) | ||
|
||
################# Feedback from FDA on need to match Pilot 1: |
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.
For the comments regarding the FDA feedback and why we're making these changes, I wonder if this needs to be included here since we're already capturing this conversation in git comments.
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.
Referring to Lines 118 - 122.
@laxamanaj, @bms63, @kaz462 - commented lined 118 - 122 removed per requested changes. thx |
tlf-primary.r has been rerun after the update, the new output matches the one generated by pilot1
There are still some differences in the dataset
adadas
due to the following issue with CDISC data (NO impact for the output):CDISC SDTM/qs: 818 records for
QSTESTCD=ACTOT
CDISC ADaM/adadas: 1040 records for
PARAMCD=ACTOT
, 799 (directly from qs, should be 818) + 241 imputed records (DTYPE=LOCF
)adadas generated by R: 1040 records for
PARAMCD=ACTOT
, 818 (directly from qs) + 222 imputed records (DTYPE=LOCF
)New diffs: