Skip to content

GDR-3366: replace hardcoded Duration values with NA_real_ #119

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

Merged
bczech merged 3 commits into from
May 5, 2026
Merged

GDR-3366: replace hardcoded Duration values with NA_real_ #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c84b01a
GDR-3366: replace hardcoded Duration values with NA_real_
bczech Apr 28, 2026
6b82007
GDR-3366: update tests to expect NA_real_ Duration in level6 PRISM
bczech Apr 28, 2026
909f69b
chore: merge main and update version to 1.11.2
bczech May 4, 2026
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
4 changes: 2 additions & 2 deletions DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
Package: gDRimport
Type: Package
Title: Package for handling the import of dose-response data
Version: 1.11.1
Date: 2026-04-29
Version: 1.11.2
Date: 2026-05-04
Authors@R: c(
person("Arkadiusz", "Gladki", role=c("aut", "cre"), email="gladki.arkadiusz@gmail.com",
comment = c(ORCID = "0000-0002-7059-6378")),
Expand Down
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## gDRimport 1.11.2 - 2026-05-04
* replace hardcoded `Duration` values with `NA_real_` in PRISM and PSet importers

## gDRimport 1.11.1 - 2026-04-29
* synchronize Bioconductor and GitHub versioning

Expand Down
4 changes: 2 additions & 2 deletions R/prism_to_gdrDF.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ convert_LEVEL6_prism_to_gDR_input <- function(prism_data_path,
Gnumber = untrt_tag,
DrugName = untrt_tag,
drug_moa = untrt_tag,
Duration = 120,
Duration = NA_real_,
Concentration = 0,
ReadoutValue = 1,
masked = FALSE)
Expand All @@ -299,7 +299,7 @@ convert_LEVEL6_prism_to_gDR_input <- function(prism_data_path,
data.table::setnames(dt_trt,
old = c("broad_id", "name", "moa", "dose", "value"),
new = c("Gnumber", "DrugName", "drug_moa", "Concentration", "ReadoutValue"))
dt_trt$Duration <- 120
dt_trt$Duration <- NA_real_
dt_trt$masked <- FALSE
data.table::setcolorder(dt_trt, neworder = colnames(dt_ctrl))

Expand Down
3 changes: 1 addition & 2 deletions R/pset_to_gdrDF.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,8 @@ getPSet <- function(pset_name,
refDivTime <- gDRutils::get_env_identifiers("cellline_ref_div_time")


# Some datasets do not have duration specified so let's assign any value, since it is required by gDR
if (!duration %in% names(dt)) {
dt[, (duration) := 72]
dt[, (duration) := NA_real_]
}
Comment on lines 205 to 207
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 28, 2026

Choose a reason for hiding this comment

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

medium

While replacing the hardcoded value with NA_real_ is correct, removing the comment entirely makes the code less self-documenting. It would be beneficial to keep a comment explaining that Duration is a required field in the gDR pipeline and is being initialized with NA_real_ when missing from the source data to maintain semantic honesty.

if (!refDivTime %in% names(dt)) {
dt[, (refDivTime) := NA]
Expand Down
4 changes: 3 additions & 1 deletion tests/testthat/test-prism_to_gdrDF.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ test_that("prism level6 data can be processed into gDR format ", {
"subtype", "ReferenceDivisionTime"))
expect_true(all(gDRutils::get_env_identifiers(c("drug", "cellline"),
simplify = FALSE) %in% names(df_prism$result)))

expect_true(all(is.na(df_prism$result$Duration)))
expect_type(df_prism$result$Duration, "double")
Comment on lines +64 to +65
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 28, 2026

Choose a reason for hiding this comment

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

medium

The added tests correctly verify that Duration is NA and of type double for Level 6 PRISM data. However, similar assertions are missing for the PSet importer tests in tests/testthat/test-pset_to_gdrDF.R. To ensure consistency and prevent regressions, consider adding equivalent checks to the PSet unit tests where the default NA_real_ assignment is expected to occur.


# testing format of clid, CellLineName and Tissue column
expect_equal(df_prism$result$clid, df_prism$result$CellLineName)
expect_equal(
Expand Down
Loading