Skip to content
New issue

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

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

Already on GitHub? # to your account

Closes #40 update pilot3 programs for zip build+run with pilot3utils namespace and library reference #41

Conversation

laxamanaj
Copy link
Collaborator

Closes #40

In testing phase.

…he most current copy from submissions-pilot3-adam repo.
Copy link
Collaborator

@kaz462 kaz462 left a comment

Choose a reason for hiding this comment

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

I tested for windows, it works fine. thanks @laxamanaj for putting things together!

@SHAESEN2
Copy link

Hey Joel, Looks good. Below some minor comments.

  1. Could you write out the 2 lines that need to be added after this instruction: "Open the .Rprofile : C:/pilot3-files/.Rprofile and ensure these 2 lines are there :" => easier to go through steps and less error prone

  2. I receive a warning :"The project is currently out-of-sync. Use renv::status() for more details." Is this something we need to acknowledge to the reviewers?
    The following package(s) do not appear to be used in this project:
    _
    DT [0.18]
    R.cache [0.16.0]
    R.methodsS3 [1.8.2]
    R.oo [1.25.0]
    R.utils [2.12.2]
    brew [1.0-8]
    brio [1.1.3]
    codetools [0.2-19]
    config [0.3.1]
    credentials [1.3.2]
    crosstalk [1.1.1]
    cyclocomp [1.1.0]
    desc [1.4.2]
    devtools [2.4.5]

  3. We need to include instruction to copy over the metadata file to the ADAM-FDA folder Error: path does not exist: ‘~/pilot3/m5/datasets/rconsortiumpilot3/analysis/adam/datasets-fda/adam-pilot-3.xlsx’

  4. Instruct them to create an output folder under pilot3/m5/datasets/rconsortiumpilot3/analysis to make sure they dont get an error when running the output programs

  5. Is it on purpose that we have '.out' extension for the demographic table?

@laxamanaj
Copy link
Collaborator Author

further feedback from team :

- Alexandra -
---- step 9.1.2 didn't create the .Rproj.user folder, but may mention that this file/folder may not be needed.
---- at the step 9.1.3, step 5, where renv selects option 1, an error reading in package 'READXL'
---- needs renv:: status showed out of sync from restart.
---- outputs generated can't open locally.
- Ben/Joel
---- update renv lock in the fda repo area instead.  that will be the one to submit.
- Kaz
---- rtf shows up fine in windows on local
---- zip file installation, when unzipping note that this is for local window OS only. not linux.

@AlexandraP-21 @bms63 @kaz462

@bms63
Copy link
Collaborator

bms63 commented Mar 29, 2024

Just to note here - the current lockfile is quering github for xportr 0.2.0 and pilot3utils 0.2.0. This is not correct.

xportr 0.2.0 should be retrieved from CRAN.

pilot3utils should be uploaded via zip method. I think this might produce a warning around the package not being in the renv.lock, but unsure.

@SHAESEN2 I believe @robertdevine recent update to the renv.lock solves your warning issue.

@laxamanaj
Copy link
Collaborator Author

Just to note here - the current lockfile is quering github for xportr 0.2.0 and pilot3utils 0.2.0. This is not correct.

xportr 0.2.0 should be retrieved from CRAN.

pilot3utils should be uploaded via zip method. I think this might produce a warning around the package not being in the renv.lock, but unsure.

@SHAESEN2 I believe @robertdevine recent update to the renv.lock solves your warning issue.

Thanks for this note here @bms63 . Upon my testing from the fda repo area. Knowing that the programs from here should have the list of packages we truly need to run these analysis, I did a snapshot of the renv.lock at time of testing. The lock file should now be updated so that {pilot3utils} is not in the lock file and that {xportr} is now coming from CRAN/RSPM instead of github.

@laxamanaj
Copy link
Collaborator Author

laxamanaj commented Mar 29, 2024

Hey Joel, Looks good. Below some minor comments.

  1. Could you write out the 2 lines that need to be added after this instruction: "Open the .Rprofile : C:/pilot3-files/.Rprofile and ensure these 2 lines are there :" => easier to go through steps and less error prone
  2. I receive a warning :"The project is currently out-of-sync. Use renv::status() for more details." Is this something we need to acknowledge to the reviewers?
    The following package(s) do not appear to be used in this project:
    _
    DT [0.18]
    R.cache [0.16.0]
    R.methodsS3 [1.8.2]
    R.oo [1.25.0]
    R.utils [2.12.2]
    brew [1.0-8]
    brio [1.1.3]
    codetools [0.2-19]
    config [0.3.1]
    credentials [1.3.2]
    crosstalk [1.1.1]
    cyclocomp [1.1.0]
    desc [1.4.2]
    devtools [2.4.5]
  3. We need to include instruction to copy over the metadata file to the ADAM-FDA folder Error: path does not exist: ‘~/pilot3/m5/datasets/rconsortiumpilot3/analysis/adam/datasets-fda/adam-pilot-3.xlsx’
  4. Instruct them to create an output folder under pilot3/m5/datasets/rconsortiumpilot3/analysis to make sure they dont get an error when running the output programs
  5. Is it on purpose that we have '.out' extension for the demographic table?

Hi, @SHAESEN2 .

Many thanks for your review as I've made updates to help resolve your feedback.

  1. Updated to include 2 lines as text.
  2. I updated renv.lock while testing in the FDA repo area so that renv will only search through our analysis *.r programs and no other files. I also included notes in the adrg to ignore the renv::status warning until after doing the renv::restore().
  3. I too received this error, so I updated these steps in the adrg as well, when updating the paths to the folders.
  4. See 3.
  5. Yes, this was mentioned by Pilot 1 team as they wanted to show that different outputs formats can be generated. So some team members created a .out, other made .rtf and others made .pdf. FDA accepted as ok for them, so let's stay consistent.

@laxamanaj
Copy link
Collaborator Author

Hi, Team.

A now more recent copy of all of the files, including update renv.lock and adrg.pdf can be found in this branch for testing. Please download updated .zip.
image

@laxamanaj
Copy link
Collaborator Author

laxamanaj commented Apr 9, 2024

Hey Joel, Looks good. Below some minor comments.

  1. Could you write out the 2 lines that need to be added after this instruction: "Open the .Rprofile : C:/pilot3-files/.Rprofile and ensure these 2 lines are there :" => easier to go through steps and less error prone
  2. I receive a warning :"The project is currently out-of-sync. Use renv::status() for more details." Is this something we need to acknowledge to the reviewers?
    The following package(s) do not appear to be used in this project:
    _
    DT [0.18]
    R.cache [0.16.0]
    R.methodsS3 [1.8.2]
    R.oo [1.25.0]
    R.utils [2.12.2]
    brew [1.0-8]
    brio [1.1.3]
    codetools [0.2-19]
    config [0.3.1]
    credentials [1.3.2]
    crosstalk [1.1.1]
    cyclocomp [1.1.0]
    desc [1.4.2]
    devtools [2.4.5]
  3. We need to include instruction to copy over the metadata file to the ADAM-FDA folder Error: path does not exist: ‘~/pilot3/m5/datasets/rconsortiumpilot3/analysis/adam/datasets-fda/adam-pilot-3.xlsx’
  4. Instruct them to create an output folder under pilot3/m5/datasets/rconsortiumpilot3/analysis to make sure they dont get an error when running the output programs
  5. Is it on purpose that we have '.out' extension for the demographic table?

Hi, @SHAESEN2 .

Many thanks for your review as I've made updates to help resolve your feedback.

  1. Updated to include 2 lines as text.
  2. I updated renv.lock while testing in the FDA repo area so that renv will only search through our analysis *.r programs and no other files. I also included notes in the adrg to ignore the renv::status warning until after doing the renv::restore().
  3. I too received this error, so I updated these steps in the adrg as well, when updating the paths to the folders.
  4. See 3.
  5. Yes, this was mentioned by Pilot 1 team as they wanted to show that different outputs formats can be generated. So some team members created a .out, other made .rtf and others made .pdf. FDA accepted as ok for them, so let's stay consistent.

Hi, @SHAESEN2 . Just wanted to confirm with you that I've resolved your findings with my updates, so we can merge?

@laxamanaj laxamanaj merged commit 236fbd1 into main Apr 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Pilot3 programs for .zip build+run with pilot3utils namespace and library reference
4 participants