Refactor finemap2anndata: major improvements in performance and memory usage#120
Refactor finemap2anndata: major improvements in performance and memory usage#120ariannalandini wants to merge 1 commit into
Conversation
…info into an anndata
edg1983
left a comment
edg1983
left a comment
There was a problem hiding this comment.
Overall, looks good. I've added some suggestions. Take a look to see if thereis anything useful to incorporate.
| suppressPackageStartupMessages(library(data.table)) | ||
| suppressPackageStartupMessages(library(anndata)) | ||
| suppressPackageStartupMessages(library(Matrix)) | ||
| suppressPackageStartupMessages(library(dplyr)) |
There was a problem hiding this comment.
I think dplyr is not needed anymore. We can remove this import and save time/memory
|
|
||
| success_count <- success_count + 1 | ||
| # Keep only SNPs belonging to the credible set | ||
| fm_cs <- fm[is_cs == TRUE] |
There was a problem hiding this comment.
This makes a full in-memory copy of the object. We can reduce memory footprint by getting the row index where is_cs == TRUE (using which) and then passing the indexes when we perform other operations.
| df = 1, | ||
| lower.tail = FALSE | ||
| ) | ||
| top_pvalue <- min(p_values, na.rm = TRUE) |
There was a problem hiding this comment.
In theory, the min p-value should equal the largest chi-square, so we can simply take
top_pvalue <- stats::pchisq(
max((fm_cs$bC / fm_cs$bC_se)^2),
df = 1,
lower.tail = FALSE
))
This avoids allocating memory for p_values vector and should be slightly faster
| position | ||
| )] | ||
| } | ||
|
|
There was a problem hiding this comment.
We may add an explicit rm(rds_obj) here to be sure the rds_obj memory is freed before allocating a new object. It would be good to test the trade-off between memory advantage and speed since cleaning takes some time (short, but still may impact if this cycle is repeated many times)
| all_snps <- snp_chr_pos$snp | ||
| labf_vec <- unlist(labf_list, use.names = FALSE) | ||
| beta_vec <- unlist(beta_list, use.names = FALSE) | ||
| se_vec <- unlist(se_list, use.names = FALSE) |
There was a problem hiding this comment.
Apparently, we collect as lists and then flatten them without any processing, right? In this case, can we just collect as a vector directly by appending the data so we avoid double memory allocation and unlist operation?
2 participants
Summary
This PR introduces a refactored implementation of
finemap2anndatafunction -finemap2anndata_fast()- that significantly improves runtime performance and memory efficiency, while preserving the original functionality.The new implementation is designed to scale to large finemapping datasets (e.g. thousands of RDS files and millions of SNPs) without exhausting memory or incurring large slowdowns.
Key Improvements
Before: loaded all RDS files into memory upfront → very high RAM usage.
Now: files are read sequentially inside the loop, memory footprint is reduced from total dataset → single file
Before: repeated assignment into sparse matrices is very slow and memory-inefficient.
lABF_matrix_sparse[row_index, col_indices] <- lABF_valuesNow: uses triplet (i, j, x) construction
Matrix::sparseMatrix(i = i_vec, j = j_vec, x = values)