Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Avoid copy when dropping attributes#483

Open
Bisaloo wants to merge 3 commits into
scverse:develscverse/anndataR:develfrom
Bisaloo:fewer-conversionBisaloo/anndataR:fewer-conversionCopy head branch name to clipboard
Open

Avoid copy when dropping attributes#483
Bisaloo wants to merge 3 commits into
scverse:develscverse/anndataR:develfrom
Bisaloo:fewer-conversionBisaloo/anndataR:fewer-conversionCopy head branch name to clipboard

Conversation

@Bisaloo

@Bisaloo Bisaloo commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

I may be missing something so mostly opening this for discussion for now.

In many cases, it seems anndataR wants a vector instead of an array with a single dimension. In the current code, this is usually achieved via as.vector() / as.integer() / etc.

But this is very inefficient because it results in a entire new copy of the object in memory.

Would it work for you to drop the dim attribute? In your use case, it should not result in an additional copy of the data.

x <- sample(1:1e7, 1e7, replace = TRUE)
dim(x) <- 1e7

bench::mark(
  as.vector(x),
  as.integer(x),
  # We assign to y here for benchmarking purposes. To avoid removing dim for our test object after the first iteration.
  { 
    y <- x
    dim(y) <- NULL
    y
  },
  iterations = 1000
)
#> # A tibble: 3 × 6
#>   expression                       min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                  <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 as.vector(x)                  11.5ms   12.3ms      81.3    38.1MB     25.3
#> 2 as.integer(x)                 11.5ms   12.3ms      81.2    38.1MB     20.3
#> 3 { y <- x dim(y) <- NULL y }    326ns    359ns 2545301.         0B      0

Created on 2026-07-20 with reprex v2.1.1

I took a reasonably large test dataset but note that this get much worse with larger datasets, while remove dim always remains free, independent of the dataset size.

Checklist

Before review

  • Update and regenerate man pages
  • Add/update tests
  • Add/update examples in vignettes
  • Pass CI checks

Before merge

  • Update NEWS
  • Bump devel version

@github-actions

github-actions Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown

🐰 Bencher Report

ProjectanndataR
Branchfewer-conversion
Testbedubuntu-latest

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 21 Alerts

🐰 View full continuous benchmarking report in Bencher

@Bisaloo Bisaloo mentioned this pull request Jul 21, 2026
Merged
@Bisaloo
Bisaloo force-pushed the fewer-conversion branch from d92bf25 to dbcf776 Compare July 21, 2026 12:27
@Bisaloo
Bisaloo force-pushed the fewer-conversion branch from dbcf776 to fd767b5 Compare July 21, 2026 13:03
@rcannood

rcannood commented Jul 22, 2026

Copy link
Copy Markdown
Member

Looks interesting! If it improves performance, I'd definitely be down! :)

@Bisaloo
Bisaloo force-pushed the fewer-conversion branch from fd767b5 to a5495d0 Compare July 22, 2026 14:44
@rcannood

rcannood commented Jul 23, 2026

Copy link
Copy Markdown
Member

LGTM when you feel this PR is ready for a review, or what additional changes you think need to be made :)

@Bisaloo

Bisaloo commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

On a 413 MB "X" layer:

cross::bench_branches(
  {
    bench::mark(
      anndataR:::read_zarr_sparse_array("adata", "X"),
      iterations = 50
    )
  },
  branches = "devel"
)
✔ Installing branch 'fewer-conversion'
✔ Installing branch 'devel'
✔ Running `expr` across variants
# A tibble: 2 × 14
Loading required namespace: Matrix
  branch           expression                                                min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result             memory     time       gc      
  <chr>            <bch:expr>                                            <bch:t> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>             <list>     <list>     <list>  
1 fewer-conversion "anndataR:::read_zarr_sparse_array(\"adata\", \"X\")"   7.13s  8.68s     0.114    17.9GB    0.629    50   276      7.32m <dgRMatrx[,19244]> <Rprofmem> <bench_tm> <tibble>
2 devel            "anndataR:::read_zarr_sparse_array(\"adata\", \"X\")"    8.3s  9.11s     0.108    17.9GB    0.558    50   258      7.71m <dgRMatrx[,19244]> <Rprofmem> <bench_tm> <tibble>

Not as huge as what I'd have hoped but I think it can still be noticeable, especially on spatialdata objects with multiple anndata components.

This is good to go as far as I'm concerned.

@Bisaloo

Bisaloo commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Related:

This step:

anndataR/R/utils.R

Line 297 in 7b7737c

data <- as.double(data)

is slow and somewhat unintuitive.

I have an X layer where the data component is nicely saved as int64. This as.double() triggers an additional, expensive, copy of the data.

But it seems that if we leave data as integer, the Matrix package emits warnings.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Bisaloo @rcannood
Morty Proxy This is a proxified and sanitized view of the page, visit original site.