Conversation
gladkia
requested review from
darsoo and
j-smola
and removed request for
a team
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the package from qs to qs2, updating dependencies, file extensions, and function calls. Feedback suggests ensuring that qs2::qs_read receives a single file path in tests to avoid potential errors when multiple files match the search pattern.
| listSE4 <- c(listSE[1], ligand = qs2::qs_read( | ||
| list.files(system.file(package = "gDRtestData", "testdata"), | ||
| "Ligand", full.names = TRUE))[[1]]) | ||
| "Ligand.*\\.qs2$", full.names = TRUE))[[1]]) |
There was a problem hiding this comment.
The call to qs2::qs_read might fail if list.files returns more than one file path or an empty vector. While the pattern is more specific now, it's safer to ensure a single path is passed, similar to how indexing is used in lines 2 and 8.
listSE4 <- c(listSE[1], ligand = qs2::qs_read(
list.files(system.file(package = "gDRtestData", "testdata"),
"Ligand.*\\.qs2$", full.names = TRUE)[1])[[1]])
There was a problem hiding this comment.
Please, update doc with devtools::document("./").
Additionally, please refactor example - instead of mae <- get_synthetic_data("finalMAE_combo_matrix_small.qs") use mae <- gDRutils::get_synthetic_data("combo_matrix_small")
There is a comment in body of convert_se_assay_to_custom_dt function with qs - maybe that also should be changed into qs2? Also description of output from process_batch function should be updated.
What with RUN Rscript -e 'BiocManager::install(c("BiocStyle", "qs"))' in DOCKERFILE?
- Dockerfile: update BiocManager install from qs to qs2 - convert_mae_se_assay_to_dt.R: update inline comments from qs to qs2 - utils.R: update process_batch @return/@details to reference .qs2 files; update get_synthetic_data @param description and all @examples to use gDRutils::get_synthetic_data("small") short-name form
bczech
left a comment
bczech
left a comment
There was a problem hiding this comment.
Please get rid of gDRutils:: namespacing within gDRutils package R/utils.R
| #' @examples | ||
| #' get_synthetic_data("finalMAE_small.qs") | ||
| #' @examples | ||
| #' gDRutils::get_synthetic_data("small") |
There was a problem hiding this comment.
This is gDRutils package so we don't need to namespace it
| #' gDRutils::get_synthetic_data("small") | |
| #' get_synthetic_data("small") |
Done, including Dockerfile update. |
j-smola
left a comment
j-smola
left a comment
There was a problem hiding this comment.
Please refactor all example - instead of mae <- get_synthetic_data("finalMAE_combo_matrix_small.qs") use mae <- gDRutils::get_synthetic_data("combo_matrix_small")
j-smola
left a comment
j-smola
left a comment
There was a problem hiding this comment.
Please, inspect carefully function description and change qs -> qs2.
Additionally, update doc with devtools::document("./").
4 participants
Description
What changed?
Related JIRA issue: GDR-3351
Replaced all usage of the archived
qspackage withqs2:qs::qread()→qs2::qs_read()qs::qsave()→qs2::qs_save().qsto.qs2Why was it changed?
The
qspackage was archived on CRAN on January 17, 2026 and will not be included in Bioconductor 3.23.The
qs2package is its official successor with an incompatible binary format.Checklist for sustainable code base
Logistic checklist