Skip to content

Refactor/output direct to h5ad#250

Open
noamteyssier wants to merge 6 commits into
dev-0.4.7from
refactor/output-direct-to-h5ad
Open

Refactor/output direct to h5ad#250
noamteyssier wants to merge 6 commits into
dev-0.4.7from
refactor/output-direct-to-h5ad

Conversation

@noamteyssier

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request replaces the external Python-based pycyto convert step with native Rust writing of .h5ad files directly from cyto-ibu-count using the anndata and anndata-hdf5 crates, eliminating the need to stage through MTX files or spawn a Python subprocess. To prevent spurious file-locking issues under concurrent Rayon execution, subprocesses for cell-filter and geomux are now invoked with HDF5_USE_FILE_LOCKING=FALSE. The review feedback focuses on improving the robustness and performance of the new write_counts_h5ad function. Specifically, it is recommended to dynamically determine the number of observations (n_obs) from the populated obs_names vector to prevent potential mismatches, and to pass the features vector by value (Vec) instead of a slice to avoid cloning the entire list of feature strings.

Comment on lines +33 to +38
let n_var = features.len();
let n_obs = counts.get_num_barcodes();
let nnz = counts.get_nnz();

let mut obs_names = Vec::with_capacity(n_obs);
let mut bc_idx_map = HashMap::with_capacity(n_obs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using counts.get_num_barcodes() as n_obs before the loop can lead to a mismatch or failure if some barcodes are filtered out or if there is any discrepancy with the actual number of unique barcodes decoded in the loop. Since obs_names is populated dynamically during the loop, it is much safer and more robust to define n_obs as obs_names.len() after the loop. We can still use counts.get_num_barcodes() to pre-allocate the capacities of obs_names and bc_idx_map.

Suggested change
let n_var = features.len();
let n_obs = counts.get_num_barcodes();
let nnz = counts.get_nnz();
let mut obs_names = Vec::with_capacity(n_obs);
let mut bc_idx_map = HashMap::with_capacity(n_obs);
let n_var = features.len();
let n_barcodes = counts.get_num_barcodes();
let nnz = counts.get_nnz();
let mut obs_names = Vec::with_capacity(n_barcodes);
let mut bc_idx_map = HashMap::with_capacity(n_barcodes);

Comment on lines +63 to +64
let coo = CooMatrix::try_from_triplets(n_obs, n_var, row_idx, col_idx, vals)
.map_err(|e| anyhow::anyhow!("Unable to build sparse count matrix: {e}"))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Use obs_names.len() as n_obs to ensure the number of rows in the sparse matrix matches the length of obs_names exactly, preventing any potential out-of-bounds errors or mismatches.

Suggested change
let coo = CooMatrix::try_from_triplets(n_obs, n_var, row_idx, col_idx, vals)
.map_err(|e| anyhow::anyhow!("Unable to build sparse count matrix: {e}"))?;
let n_obs = obs_names.len();
let coo = CooMatrix::try_from_triplets(n_obs, n_var, row_idx, col_idx, vals)
.map_err(|e| anyhow::anyhow!("Unable to build sparse count matrix: {e}"))?;

Comment on lines +21 to +27
pub fn write_counts_h5ad<P: AsRef<Path>>(
path: P,
counts: &BarcodeIndexCounts,
features: &[String],
header: Header,
suffix: Option<&str>,
) -> Result<()> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To avoid cloning the entire vector of feature strings (which can contain tens of thousands of elements), we can pass features by value as Vec<String> instead of a slice &[String]. Since write_counts_h5ad is the final step in the counting process and consumes the features, passing by value is highly efficient and avoids unnecessary heap allocations.

Suggested change
pub fn write_counts_h5ad<P: AsRef<Path>>(
path: P,
counts: &BarcodeIndexCounts,
features: &[String],
header: Header,
suffix: Option<&str>,
) -> Result<()> {
pub fn write_counts_h5ad<P: AsRef<Path>>(
path: P,
counts: &BarcodeIndexCounts,
features: Vec<String>,
header: Header,
suffix: Option<&str>,
) -> Result<()> {

let adata = AnnData::<H5>::new(path.as_ref())?;
adata.set_x(csr)?;
adata.set_obs_names(DataFrameIndex::from(obs_names))?;
adata.set_var_names(DataFrameIndex::from(features.to_vec()))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since features is now owned by the function, we can pass it directly to DataFrameIndex::from without calling .to_vec(), completely eliminating the need to clone the feature strings.

Suggested change
adata.set_var_names(DataFrameIndex::from(features.to_vec()))?;
adata.set_var_names(DataFrameIndex::from(features))?;

Comment on lines +337 to +339
features
.expect("Must provide a feature file to write h5ad")
.as_slice(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Pass features by value to write_counts_h5ad instead of taking a slice, to avoid cloning the feature strings inside the function.

            features
                .expect("Must provide a feature file to write h5ad"),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant