-
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
Tidying tlf-demographic.Rmd removing {pilot1wrappers} package dependency. #178
Conversation
…ncy. This change does not affect the lock file.
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.
Does it need pilot3utils to run?
No, this is backlog related historically in Pilot3 to Agency Comment 3. The {pilot1wrappers} reference dependency remained from when the {pilot3} name was used prior to the Pilot3 Team addressing the agency comments. Checking the other vignettes (and non-submission articles for any other strays). Thx. Checking the programs and outputs with the Pilot 3 Team responses and if all goes well will refresh tlf-report and clean up the (non-submission) vignettes as well of the {pilot1wrappers} reference. In the vignettes (demographic, efficacy} the {pilotwrappers} references get removed and the namespaces are referenced as {pilot3utils}. |
…dating namespace references to {pilot3utils}. This change does not affect the lock file.
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.
Many thanks for this @robertdevine . Looks good to me. Did the other output programs need the call to pilot3utils as well?
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.
Approve otherwsie, if these were the only 2 programs that needed to be updated. Thanks for this.
…dating namespace references to {pilot3utils} for nest_rowlabels( ) in primary output. This change does not affect the lock file.
Yes, @laxamanaj , the Pilot3 assembly is now free of the {pilot1wrappers} dependencies and namespace overlaps with the former {pilot3} naming. Per the Agency feedback, the proprietary package is named {pilot3utils}. The vignettes have been updated removing the library references to {pilot1wrappers} and the namespaces for the function calls has been changed from {pilot1wrappers} namespace to {pilot3utils} namespace. So, in summary, if the .zip archive installation instructions are used the programs and outputs run correctly and the tlf-report.pdf with refreshed timestamps is generated. Should be all set. Thanks. |
A bit of history may help explain, @bms63 Pilot1 (OS: ubuntu v20.04) used libreoffice to generate the primary endpoint analysis For purposes of consistency, Pilot3 did the same back in August. The cloud service provider On Friday I did confirm above the process used by Pilot3 to generate the tlf-report.pdf For consistency, the same process to re-fresh timestamps should be followed for the re-submission. The >soffice --convert-to pdf [command is used and works with primary and efficacy rtf-to-pdf.] Lastly, Pilot3 also innovated other techniques to perform this conversion including the browser engine - anyone Should be all set on this. item. @laxamanaj please let me know if you need me to upload the refreshed report Thanks. |
Awesome. So we just need to rerun the code to get the refreshed report. @laxamanaj explained this to me a bit more on Firday. I was confused and I thought this PR was going to generate it, but now I understand he needs to re-run it. Do we need to document or put instructions anywhere? If there isn't any...?? |
Up to you @bms63 and @laxamanaj, if so, please create the issue and I'll self-assign to write up a document. Thx. |
I think we should note the process somewhere? I'm not sure where - maybe just in the related |
@bms63, @laxamanaj - will document the process here. thx. |
Hi, @robertdevine . Many thanks for generating the report. I believe at this time, any one of us could regenerate this. I've already refreshed all of the ADaMs and outputs as of 12Apr2024 (https://github.com/RConsortium/submissions-pilot3-adam/tree/main/submission/output) . Though, I did not convert the .rtf to .pdf to generate the the |
…report-tlf-pilot3.pdf
Hi, @bms63 , @kaz462 , @SHAESEN2 , @AlexandraP-21 and @robertdevine . As my last commit message says, I've rerun the outputs with today's timestamps and did a manual combine of the outputs into pdf as our current issue we have yet to resolve is the automated rtf to pdf conversion. Though the latter has no impact on the results, just a formatting issue which can be resolved at a later time (i.e. local libreoffice install, then run this function : https://merck.github.io/r2rtf/articles/rtf-convert-format.html ). To note here, these are the steps I took.
Please review this new pdf in step 6 and compare against this output in Pilot 1 https://github.com/RConsortium/submissions-pilot1-to-fda/blob/main/m1/us/report-tlf.pdf. All results should match. Upon your review, if these match up, then this branch can now be merged into main to close out. Then we can just download the final output in step 6 and bring over to the pilot 3 adam to fda repo. Best, |
Thanks @laxamanaj - very cool. As noted here, >soffice --convert-to pdf command (in libreoffice) was The tlf-report pdf looks good, @laxamanaj - however the pilot3-tlf-report can be generated programmatically Do we know if the agency has a preference? Pilot3 can certainly do this programmatically by updating the .Rmd The Merck wrapper I had not tested - so I would test that and push the program to auto-generate pilot3-tlf-report.pdf |
Thanks @robertdevine . Hoping to get feedback that the results from the current report-tlf-pilot3.pdf I generated still matches the pilot1 report-tlf.pdf, which I've linked above. I just need a second pair of eyes to confirm in a comment, then I can merge. Or if the results match to you @bms63 then please merge as soon as you complete review. Thanks. |
@laxamanaj - under the hood the two pdf files differ widely. A simple diff of the pdf's shows the text and pagination differences immediately. If your using Mac (BBEdit) or Windows (WinMerge), the differences show. Just an eye scan the documents appear closely matched with three areas of core differences: (1)pagination, (2)text, (verbiage), and (3) inconsistent significant digits in tabular values in the pilot1 vs. pilot3 pdf's. For tabular data pilot3 two significant digits vs. one significant digit in pilot1 (please see below). Should there be agreement in tabular reported accuracy for sake of scientific consistency between the pdf's in the agency submissions - based upon agency review to date likely advisable. Or does the two-significant digit consistency in the Pilot3 tables meet the agency expectation for the tabular data. The two pdf's were generated using differing technologies pilot1 (Google PDF Generator) and pilot3 (Adobe). Reasonable to expect for an eye-level for tlf report match as long as the pagination matches unless otherwise told, slight differences in text are to be expected (see attached) given differing pilots, and the table and figures should match exactly. Comparing the guts of the two pdf documents they differ widely but that should be fine given the different technologies used to generate the pdf's in each pilot. Reasonable if @laxamanaj and @bms63 approve the differences then merge is fine, however, given agency review to date, pdf difference noted as (3) inconsistencies in table value significant digits comparing pilot 1 and pilot3 is likely a concern that should be addressed. LibreOffice or Adobe Acrobat can also compare the pdf's. report-tlf-p1-diff-pilot3-report-tlf-pilot3.pdf Other than the above, @laxamanaj , @bms63 - good to go on the merge. Seems like minor diffs but (3) looks like a cause for concern based on agency feedback Pilot 3 Team has received from our regulatory review panel members so far. Thx. |
This change does not affect the lock file. Re-testing output before pushing report-tlf.pdf with refreshed timestamps. thx.