Closes #40 use pharmaversesdtm to host dm_peds and vs_peds #84

zdz2101 · 2024-08-02T16:26:05Z

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

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. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the main branch until you have checked off each task.

github-actions · 2024-08-02T16:29:42Z

Package	Line Rate	Health
admiralpeds	99%	✔
Summary	99% (267 / 271)	✔

zdz2101 · 2024-08-02T18:13:59Z

Shouldn't actually be necessary to merge and close this but to demo the migration assuming the goes through this is done

…iralpeds_data_migration

rossfarrugia · 2024-08-23T10:42:14Z

DESCRIPTION

    rmarkdown,
    stringr (>= 1.4.0),
    testthat (>= 3.0.0),
    tibble
+Remotes: pharmaverse/pharmaversesdtm@admiralpeds_data_migration


@zdz2101 this bit can be removed and you could get this and pharmaverse/pharmaversesdtm#115 ready for review now, and think should be good timing for me and @Fanny-Gautier to take a look in the coming weeks

I think the steps go:

merge the pharmaversesdtm PR first

remove this line

see if this PR still works with the removal of pharmaversdtm:: as well, because I think it'll still rely on a remotes until the pharmaversesdtm package is re-released to a new version

Plus Step 4) add an adsl_peds and advs_peds to pharmaverseadam. we can make an issue for this later after all these steps

As for adsl_peds and advs_peds it seems there was some ongoing discussion about what is appropriate to house in there as the datasets for that package are made from the templates in admiral see discussion from this issue

I think they mean any admiral package templates incl extensions here, as they do have ADaMs created from ophtha, onco, vaccine templates for example at https://github.com/pharmaverse/pharmaverseadam/tree/main/data

rossfarrugia · 2024-08-23T10:43:07Z

R/derive_params_growth_age.R

 #'
-#' advs <- dm_peds %>%
+#' advs <- pharmaversesdtm::dm_peds %>%


should be able to remove pharmaversesdtm:: - i don't think we usually include these. i guess this was just for testing purposes

to demonstrate it works being in pharmaversesdtm if its actually in there, yes

Fanny-Gautier

2 minor comments in the vignette and template.
Also, does it require a notification in NEWS.md ?

Fanny-Gautier · 2024-09-09T14:33:22Z

inst/templates/ad_advs.R

@@ -180,7 +181,7 @@ who_wt_for_lgth <- who_wt_for_lgth_boys %>%
 # as needed and assign to the variables below.
 # For illustration purposes read in admiral test data

-data("vs_peds")
+vs_peds <- vs_peds


Is this line of code necessary ?

you're probably right that this could be removed

@zdz2101 don't we still need the data("xxx") lines though to load the data from the pharmaversesdtm package? see https://github.com/pharmaverse/admiral/blob/main/inst/templates/ad_advs.R for example (lines 18 and 19)

Also agree we should have a NEWS.md entry for this change - if you merge latest main branch here i've started off the headers for this for next release

I guess that data() has been dropped as per following discussion.
I was only wondering whether we need vs_peds <- vs_peds, or make it clear in a comment where it comes from since removing data(vs_peds) could be confusing.

ah thanks, sorry i had missed that latest admiral core issue/discussion around replacing the data(xxx) usage

Fanny-Gautier · 2024-09-09T14:34:53Z

vignettes/advs.Rmd

@@ -230,7 +232,7 @@ include the pediatrics-specific parameters as needed - for example:
 | BMIPCTL   | BMI-for-age percentile | 10       | Subject Characteristic | 1          |

 ```{r echo=FALSE, message=FALSE}
-data("vs_peds")
+vs_peds <- vs_peds


Is this line of code really necessary ?

…iralpeds_data_migration

zdz2101 · 2024-09-10T15:45:50Z

@Fanny-Gautier @rossfarrugia made the appropriate updates that use the :: convention that was in the admiral discussion

rossfarrugia · 2024-10-01T10:17:29Z

@zdz2101 we should add something to NEWS file in this PR. I think Fanny is busy with project work so if you want to merge pharmaverse/pharmaversesdtm#115, and then we can get this one merged too and ticked off the list.

Fanny-Gautier · 2024-10-01T10:44:17Z

@zdz2101 we should add something to NEWS file in this PR. I think Fanny is busy with project work so if you want to merge pharmaverse/pharmaversesdtm#115, and then we can get this one merged too and ticked off the list.

Sorry for not having followed much these last few days. I am currently over busy with Study work and the R-Pharma workshop preparation. Please do not hesitate to ping me in Slack for any request which require my attention... I'll add them on my to do list.
In the mean time, I'l add this PR review on my list for tomorrow ;-) Thanks

Fanny-Gautier

I think there have been some confusion while calling the data in the templates/ad_advs.R. Please double check and update accordingly.

Fanny-Gautier · 2024-10-02T08:59:48Z

inst/templates/ad_advs.R

-data(who_lgth_ht_for_age_boys)
-data(who_lgth_ht_for_age_girls)
-data(cdc_htage)
+who_lgth_ht_for_age_boys <- admiralpeds::who_wt_for_age_boys


Didn't you reverse the calls at lines 74 with lines 115 ?
e.g. should be who_lgth_ht_for_age_boys <- admiralpeds::who_lgth_ht_for_age_boys

Fanny-Gautier · 2024-10-02T09:00:53Z

inst/templates/ad_advs.R

-data(who_lgth_ht_for_age_girls)
-data(cdc_htage)
+who_lgth_ht_for_age_boys <- admiralpeds::who_wt_for_age_boys
+who_lgth_ht_for_age_girls <- admiralpeds::who_wt_for_age_girls


Didn't you reverse the calls at lines 75 with lines 116 ?
e.g. should be who_lgth_ht_for_age_girls <-admiralpeds::who_lgth_ht_for_age_girls

Fanny-Gautier · 2024-10-02T09:01:53Z

inst/templates/ad_advs.R

-data(who_wt_for_age_boys)
-data(who_wt_for_age_girls)
-data(cdc_wtage)
+who_wt_for_age_boys <- admiralpeds::who_lgth_ht_for_age_boys


Didn't you reverse the calls at lines 74 with lines 115 ?
e.g. should be who_wt_for_age_boys <- admiralpeds::who_wt_for_age_boys

Fanny-Gautier · 2024-10-02T09:03:10Z

inst/templates/ad_advs.R

-data(who_wt_for_age_girls)
-data(cdc_wtage)
+who_wt_for_age_boys <- admiralpeds::who_lgth_ht_for_age_boys
+who_wt_for_age_girls <- admiralpeds::who_lgth_ht_for_age_girls


Didn't you reverse the calls at lines 75 with lines 116 ?
e.g. should be who_wt_for_age_girls <- admiralpeds::who_wt_for_age_girls

NEWS.md

Co-authored-by: Ross Farrugia <82581364+rossfarrugia@users.noreply.github.com>

Fanny-Gautier

Looks good now.

rossfarrugia · 2024-10-16T08:45:21Z

Great - thanks @Fanny-Gautier ! @zdz2101 when you get chance are you ok to merge and do a quick test run of main now that {pharmaversesdtm} is updated too. Then the final step would be making an issue for adsl_peds and advs_peds in {pharmaverseadam}.

need staged dependencies to check everything still works

1d2557b

zdz2101 added 4 commits August 2, 2024 17:25

demo pharmaversesdtm works, if properly installed

3c79abc

check if this works

45f0aaa

needs to be in import

4bca9ef

can it be suggests?

4eed445

zdz2101 requested review from Fanny-Gautier and rossfarrugia August 2, 2024 18:12

zdz2101 marked this pull request as ready for review August 2, 2024 18:13

rossfarrugia linked an issue Aug 5, 2024 that may be closed by this pull request

Move test data out to {pharmaversesdtm} and {pharmaverseadam} #40

Closed

Merge branch 'main' into 40-move-test-data-out-to-pharmaversesdtm@adm…

935eec8

…iralpeds_data_migration

rossfarrugia reviewed Aug 23, 2024

View reviewed changes

zdz2101 added 4 commits August 23, 2024 19:14

remove pharmaversesdtm:: reference

1c4d8f4

can't be removed yet

15972ab

does this work?

4969902

typo

fc90f51

Fanny-Gautier reviewed Sep 9, 2024

View reviewed changes

zdz2101 added 2 commits September 10, 2024 15:39

use :: convention

a456a5b

Merge branch 'main' into 40-move-test-data-out-to-pharmaversesdtm@adm…

f248704

…iralpeds_data_migration

zdz2101 added 3 commits September 16, 2024 19:36

only use :: formatting

2cd69a7

missed a spot

5623fe6

missed one

716143e

Fanny-Gautier requested changes Oct 2, 2024

View reviewed changes

zdz2101 added 2 commits October 7, 2024 22:16

slight inconsistencies are fixed

37da7e7

add news entry

fcffc5d

rossfarrugia reviewed Oct 9, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

zdz2101 and others added 2 commits October 9, 2024 09:41

Update NEWS.md

6f53497

Co-authored-by: Ross Farrugia <82581364+rossfarrugia@users.noreply.github.com>

fix error

01db875

Fanny-Gautier approved these changes Oct 16, 2024

View reviewed changes

zdz2101 merged commit 627aaf7 into main Oct 16, 2024
15 of 16 checks passed

zdz2101 deleted the 40-move-test-data-out-to-pharmaversesdtm@admiralpeds_data_migration branch October 16, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #40 use pharmaversesdtm to host dm_peds and vs_peds #84

Closes #40 use pharmaversesdtm to host dm_peds and vs_peds #84

zdz2101 commented Aug 2, 2024

github-actions bot commented Aug 2, 2024 •

edited

Loading

zdz2101 commented Aug 2, 2024

rossfarrugia Aug 23, 2024

zdz2101 Aug 23, 2024 •

edited

Loading

rossfarrugia Aug 28, 2024

zdz2101 Aug 28, 2024

rossfarrugia Aug 29, 2024 •

edited

Loading

rossfarrugia Aug 23, 2024

zdz2101 Aug 23, 2024

Fanny-Gautier left a comment

Fanny-Gautier Sep 9, 2024

rossfarrugia Sep 10, 2024

rossfarrugia Sep 10, 2024

rossfarrugia Sep 10, 2024

Fanny-Gautier Sep 10, 2024

rossfarrugia Sep 10, 2024

Fanny-Gautier Sep 9, 2024

zdz2101 commented Sep 10, 2024

rossfarrugia commented Oct 1, 2024

Fanny-Gautier commented Oct 1, 2024

Fanny-Gautier left a comment

Fanny-Gautier Oct 2, 2024

Fanny-Gautier Oct 2, 2024

Fanny-Gautier Oct 2, 2024

Fanny-Gautier Oct 2, 2024

Fanny-Gautier left a comment

rossfarrugia commented Oct 16, 2024

Closes #40 use pharmaversesdtm to host dm_peds and vs_peds #84

Closes #40 use pharmaversesdtm to host dm_peds and vs_peds #84

Conversation

zdz2101 commented Aug 2, 2024

github-actions bot commented Aug 2, 2024 • edited Loading

zdz2101 commented Aug 2, 2024

Choose a reason for hiding this comment

zdz2101 Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossfarrugia Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fanny-Gautier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdz2101 commented Sep 10, 2024

rossfarrugia commented Oct 1, 2024

Fanny-Gautier commented Oct 1, 2024

Fanny-Gautier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fanny-Gautier left a comment

Choose a reason for hiding this comment

rossfarrugia commented Oct 16, 2024

github-actions bot commented Aug 2, 2024 •

edited

Loading

zdz2101 Aug 23, 2024 •

edited

Loading

rossfarrugia Aug 29, 2024 •

edited

Loading