Skip to content

Conversation

@keller-mark
Copy link

@keller-mark keller-mark commented Nov 5, 2024

Fixes #91

These changes are from both me and @Artur-man

The main public-facing changes here are:

  • The ZarrAnnData class
  • read_zarr and write_zarr top-level functions
  • Support for from_Seurat(output_class="ZarrAnnData")
  • Support for from_SingleCellExperiment(output_class="ZarrAnnData")

Internally:

  • read_zarr_helpers.R is the zarr analog of read_h5ad_helpers.R
  • write_zarr_helpers.R is the zarr analog of write_h5ad_helpers.R
  • Test fixtures within inst/extdata/example.zarr (this makes the diff noisy, apologies)
  • Lots of tests:
    • test-Zarr-read.R (35 new tests)
    • test-Zarr-write.R (70)
    • test-ZarrAnnData.R (26)
    • test-h5ad-zarr.R (17)

A number of these functions generate warnings in the R console that are intended to be followed up on to improve the code (and should probably be resolved as end users may not appreciate them), but the tests still pass despite these warnings.

Known things that are not implemented here:

  • support for recarrays
  • usage of mode = c("r", "r+", "a", "w", "w-", "x") parameter value

Copy link
Member

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Fantastic work @keller-mark and @Artur-man !

I went through the PR for a first time and left some minor comments. I will review the code by running it a couple of times next :)

attrs <- g$get_attrs()$to_list()

if (!all(c("encoding-type", "encoding-version") %in% names(attrs))) {
path <- name
Copy link
Member

Choose a reason for hiding this comment

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

where are a lot of linting issues in this file -- could you run lintr::lint_package() and fix any issues that pop up?

Choose a reason for hiding this comment

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

done ... did a full lint_package check and corrected some R check issues too!

Copy link
Member

Choose a reason for hiding this comment

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

This file should probably be removed

Choose a reason for hiding this comment

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

done

@Artur-man
Copy link

Errors I am getting at lint and pkgdown GHAs look unrelated to the package:

Error in dyn.load(file, DLLpath = DLLpath, ...) : 
    unable to load shared object '/home/runner/work/_temp/Library/Rhdf5lib/libs/Rhdf5lib.so':
    libsz.so.2: cannot open shared object file: No such file or directory

@Artur-man
Copy link

Artur-man commented Dec 21, 2025

@Bisaloo also the tests that are passing for macos, ubuntu and windows are failing for macos (intel) and ubuntu-bioc-release. I understand the error in release (for obvious reasons), but macos-15 I don't know. Any ideas ?

Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
   5.         └─base::array(metadata$fill_value, dim = lengths(index))
  ── Error ('test-Zarr-read.R:87:3'): reading numeric scalars works ──────────────
  Error in `array(metadata$fill_value, dim = lengths(index))`: 'dim' cannot be of length 0
  Backtrace:
      ▆
   1. └─anndataR:::read_zarr_numeric_scalar(store, "uns/IntScalar") at test-Zarr-read.R:87:3
   2.   └─anndataR:::read_zarr_array(store, name)
   3.     └─Rarr::read_zarr_array(file.path(store, name))
   4.       └─Rarr:::read_data(...)
   5.         └─base::array(metadata$fill_value, dim = lengths(index))
  
  [ FAIL 4 | WARN 447 | SKIP 12 | PASS 1842 ]

https://github.com/scverse/anndataR/actions/runs/20411185095/job/58648259569?pr=190

@Bisaloo
Copy link
Contributor

Bisaloo commented Dec 21, 2025

Yes, it looks that on this runner, Rarr 1.11.9 is installed, when support for reading scalars was added in Rarr 1.11.12. I think it's related to BioC devel not having more recent binaries for macOS x86 yet.

Maybe try setting:

Imports:
    Rarr (>= 1.11.12)

In DESCRIPTION to force installation from source?

This will not work for the runner using BioC release but it's already failing anyways.

@Artur-man
Copy link

I see. Then I will assume all checks are passing for now. thanks man

@Artur-man
Copy link

Artur-man commented Dec 21, 2025

Current issues:

expect_equal(array_h5ad, array_zarr)
})

test_that("reading mappings is same for h5ad and zarr", {
Copy link

@Artur-man Artur-man Jan 21, 2026

Choose a reason for hiding this comment

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

There is a difference between rhdf5, Rarr and h5py on how rec arrays and structured data types are read.

To me, h5py and Rarr reads it correctly, but rhdf5 does not:
Huber-group-EMBL/rhdf5#192

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we need an issue for this here as well? From our point of view it doesn't matter so much what is returned by {rhdf5} as long as what we return from the AnnData matches Python.

Going the other way (writing a recarray) is more difficult because we don't know when to use that format. I don't think we test this. Also, I don't think the AnnData disk spec really covers alternative arrays like recarray.

@Artur-man
Copy link

We are really close, tests are passing on my local but we have to wait until changes in Rarr are in BioC-devel.
Let me give a break I will review the entire PR. Please also let me know if you catch stuff, otherwise I will get back to this to finalize.

Thanks @Bisaloo again for answering all issues of Rarr!

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.

Implement Zarr backend

5 participants