Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 19 commits into from
Oct 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1d2557b
need staged dependencies to check everything still works
zdz2101 Aug 1, 2024
3c79abc
demo pharmaversesdtm works, if properly installed
zdz2101 Aug 2, 2024
45f0aaa
check if this works
zdz2101 Aug 2, 2024
4bca9ef
needs to be in import
zdz2101 Aug 2, 2024
4eed445
can it be suggests?
zdz2101 Aug 2, 2024
935eec8
Merge branch 'main' into 40-move-test-data-out-to-pharmaversesdtm@adm…
rossfarrugia Aug 23, 2024
1c4d8f4
remove pharmaversesdtm:: reference
zdz2101 Aug 23, 2024
15972ab
can't be removed yet
zdz2101 Aug 23, 2024
4969902
does this work?
zdz2101 Aug 23, 2024
fc90f51
typo
zdz2101 Aug 23, 2024
a456a5b
use :: convention
zdz2101 Sep 10, 2024
f248704
Merge branch 'main' into 40-move-test-data-out-to-pharmaversesdtm@adm…
zdz2101 Sep 10, 2024
2cd69a7
only use :: formatting
zdz2101 Sep 16, 2024
5623fe6
missed a spot
zdz2101 Sep 16, 2024
716143e
missed one
zdz2101 Sep 16, 2024
37da7e7
slight inconsistencies are fixed
zdz2101 Oct 7, 2024
fcffc5d
add news entry
zdz2101 Oct 7, 2024
6f53497
Update NEWS.md
zdz2101 Oct 9, 2024
01db875
fix error
zdz2101 Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,12 @@ Imports:
Suggests:
knitr,
lubridate (>= 1.7.4),
pharmaversesdtm (>= 0.2.0),
pharmaversesdtm,
rmarkdown,
stringr (>= 1.4.0),
testthat (>= 3.0.0),
tibble
Remotes: pharmaverse/pharmaversesdtm@admiralpeds_data_migration
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator Author

@zdz2101 zdz2101 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the steps go:

  1. merge the pharmaversesdtm PR first
  2. remove this line
  3. 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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@rossfarrugia rossfarrugia Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

VignetteBuilder:
knitr
Config/testthat/edition: 3
Expand Down
14 changes: 0 additions & 14 deletions R/data.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,20 +193,6 @@
#' @source \url{https://www.cdc.gov/growthcharts/percentile_data_files.htm}
"cdc_bmiage"

#' Demographic Dataset-pediatrics
#'
#' An updated SDTM DM dataset with pediatric patients
#' @keywords datasets
#' @family datasets
"dm_peds"

#' Vital signs Dataset-pediatrics
#'
#' An updated SDTM VS dataset with anthropometric measurements for pediatric patients
#' @keywords datasets
#' @family datasets
"vs_peds"

#' Subject Level Analysis Dataset-pediatrics
#'
#' An updated ADaM ADSL dataset with pediatric patients
Expand Down
1 change: 1 addition & 0 deletions R/derive_params_growth_age.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
#' library(lubridate, warn.conflicts = FALSE)
#' library(rlang, warn.conflicts = FALSE)
#' library(admiral, warn.conflicts = FALSE)
#' library(pharmaversesdtm, warn.conflicts = FALSE)
#'
#' advs <- dm_peds %>%
#' select(USUBJID, BRTHDTC, SEX) %>%
Expand Down
1 change: 1 addition & 0 deletions R/derive_params_growth_height.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
#' library(lubridate, warn.conflicts = FALSE)
#' library(rlang, warn.conflicts = FALSE)
#' library(admiral, warn.conflicts = FALSE)
#' library(pharmaversesdtm, warn.conflicts = FALSE)
#'
#' # derive weight for height/length only for those under 2 years old using WHO
#' # weight for length reference file
Expand Down
4 changes: 2 additions & 2 deletions data-raw/adsl_peds.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ library(stringr)
# Create a basic ADSL for pediatrics ----

# Read in input data ----
data("dm_peds")
data("ex")
dm_peds <- dm_peds
ex <- ex

# Derivations ----

Expand Down
47 changes: 0 additions & 47 deletions data-raw/dm_peds.R
View file
Open in desktop

This file was deleted.

262 changes: 0 additions & 262 deletions data-raw/vs_peds.R
View file
Open in desktop

This file was deleted.

Binary file removed data/dm_peds.rda
View file
Open in desktop
Binary file not shown.
Binary file removed data/vs_peds.rda
View file
Open in desktop
Binary file not shown.
3 changes: 2 additions & 1 deletion inst/templates/ad_advs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
# who_wt_for_lgth_boys, who_wt_for_lgth_girls

library(admiral)
library(pharmaversesdtm)
library(admiralpeds)
library(dplyr)
library(lubridate)
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line of code necessary ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you're probably right that this could be removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

data("adsl_peds")

vs <- vs_peds
Expand Down
Loading
Loading