82 adlbc derivations - Githubissues

RConsortium / submissions-pilot3-adam

Development repo for pilot3 submission to FDA - ADaM

https://rconsortium.github.io/submissions-pilot3-adam/

GNU General Public License v3.0

16 stars 11 forks source link

82 adlbc derivations #92

Closed nicolekjones closed 1 year ago

nicolekjones commented 1 year ago

Hi all,

I did a bit of cleaning of the code

there was an udpate to the adlbc ADT derivatoin chaingin vars() to exprs() which caused the program to break in my area
ADTM was derived, but not used in final dataset so I removed derivation
added @DadongZ 's fix for calculating maxALBTRVAL for AENTMTFL

Specs were updated

I removed any other ADLB datasets
Updated origins and provided predecessors, methods and verified the codelists
It would be great to have one more set of eyes to make sure I'm following this correctly

laxamanaj commented 1 year ago

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 :

git checkout main
git pull
git checkout [your branch]
git merge --no-edit main

then run renv::restore() in console

Before you made the update to the ADLBC specs above?

nicolekjones commented 1 year ago

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 :
git checkout main
git pull
git checkout [your branch]
git merge --no-edit main

then run renv::restore() in console
Before you made the update to the ADLBC specs above?

I thin i missed that last step. I did:

git checkout main git pull main git checkout -b

I believe there was a merge that happened between creating the branch and my final push I’ll resolve this afternoon 😁

laxamanaj commented 1 year ago

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 :
git checkout main
git pull
git checkout [your branch]
git merge --no-edit main

then run renv::restore() in console
Before you made the update to the ADLBC specs above?
I thin i missed that last step. I did:

git checkout main git pull main git checkout -b

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. :)

nicolekjones commented 1 year ago

@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. I think we've done a good job so far :)

nicolekjones commented 1 year ago

@laxamanaj, just pushed the updates. For those full_joins yes we do expect a one to many merge. updated so the warning is suppressed!

laxamanaj commented 1 year ago

Many thanks for these swift updates, @nicolekjones . I will merge now.