pharmaverse / admiral.test

Test SDTM data for use with admiral
Other
2 stars 1 forks source link

Closes #103 103 update_tr: update TR, TU, and RS #112

Closed bundfussr closed 1 year ago

bundfussr commented 1 year ago

Thank you for your Pull Request!

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request.

bms63 commented 1 year ago

Did the ungroup for admiral_tr get completed here? #98

bundfussr commented 1 year ago

@bms63 , yes, this should be fixed here.

fshanlee commented 1 year ago

@bundfussr , please could you resolve the merge conflicts?

bms63 commented 1 year ago

Hi @bundfussr , I'm assuming that after you updated the code, you ran tr.R, then subsequently re-ran rs.R and tu.R: is that correct? The code looks good to me, but I have no experience of oncology data, so I would prefer that someone who is familiar with oncology data actually approves this pull request.

Hey @fshanlee You can use this as a guide for updating the instructions for #106

bms63 commented 1 year ago

@bundfussr I was told only the labels changed, but it might be prudent to make sure data is still the same. apologies this conflicted with your PR.

bundfussr commented 1 year ago

@fshanlee , I have fixed the conflicts and recreated the datasets.

I would keep the PR open while working on the ADTR vignette. Just in case we need to change something.

fshanlee commented 1 year ago

Hi @bundfussr , in lines 7 to 13 of dev/tu.R, I think tr will be created based on the previous version of admiral_tr, not the version that has been created as part of this pull request by running tr.R. I tested this by deleting data/admiral_tr.rda from my local repo, and the code still ran, whereas if I try to explicitly load data/admiral_tr.rda


> load("data/admiral_tr.rda")
Error in readChar(con, 5L, useBytes = TRUE) : cannot open the connection
In addition: Warning message:
In readChar(con, 5L, useBytes = TRUE) :
  cannot open compressed file 'data/admiral_tr.rda', probable reason 'No such file or directory'
bundfussr commented 1 year ago

@fshanlee , good point. I have removed the library(admiral.test) calls.

fshanlee commented 1 year ago

@bundfussr , I just wondered: would it be worth adding comments at the start of tu.R and rs.R, to suggest first running tr.R to ensure that the most recent version of admiral_tr is used as input?

bms63 commented 1 year ago

Hey @bundfussr. Possible to finalize this today or tomorrow? Just want to focus on admiral for the remaining week if possible. Lots of bits and bobs!

bundfussr commented 1 year ago

@fshanlee , good idea. I have added a note to the scripts.

bundfussr commented 1 year ago

Hey @bundfussr. Possible to finalize this today or tomorrow? Just want to focus on admiral for the remaining week if possible. Lots of bits and bobs!

@bms63 , we have an admiralonco meeting tomorrow. Is it OK to finalize it thereafter.

bms63 commented 1 year ago

yes np!!

bundfussr commented 1 year ago

@bundfussr Just trying to understand updates in the TR. I used the admiral.test , 103_update_tr@devel branch to check the data. Hope this is correct. so, if TRLOC- LYMPH NODE then you used LDIAM, LPERP records. and TRSTRESN for DIAMETER test then in this LYMPH NODE should be the value of TRSTRESN of LPERP. so , manually checked 01-701-1015 subject and TRLOC is in SUPPTR. for T01 is not a lymph node even then TRSTRESN for DIAMETER is value of LPERP. as the SUPP TRLOC T02 lesion is LYMPH NODE. So, just want to make sure. what you are trying to do..

@amitjaingsk , in the vignette we use LPERP for lesions with TRLOC == "LYMPH NODE" and LDIAM otherwise. In the test data the values for DIAMETER are generated randomly first. Then the values for LPERP and LDIAM are generated by using the DIAMETER values and change some of them slightly. I want to achieve that LPERP and LDIAM are not equal but also not completely different. By the way, I noticed that I did the modifications the wrong way round. This could result in inconsistencies between PD in RS and PD derived from TR. I have fixed it.

bundfussr commented 1 year ago

@HinalPat , I did a small update to avoid inconsistencies between PD in RS and PD derived from TR. Could you reapprove?

bundfussr commented 1 year ago

@bms63 , Hinal has approved this PR but merging is still blocked. I assume she doesn't have write access. Could you give her write access or approve the PR?

bms63 commented 1 year ago

@HinalPat only has write access. @bundfussr I gave you admin access so you can merge later on.