Write obs_names/var_names as the index of obsm/varm data frames - #501
Write obs_names/var_names as the index of obsm/varm data frames#501mffrank wants to merge 4 commits into
Conversation
A `data.frame` always has row names, so the `rownames(mat) <- NULL` that
`.validate_aligned_array()` uses to strip names before storage silently
replaces them with R's automatic 1:nrow sequence rather than removing
them. `write_h5ad_data_frame()` then wrote that sequence as the on-disk
`_index`, so an `obsm`/`varm` `data.frame` ended up indexed "1".."n"
instead of by the parent's `obs_names`/`var_names`.
Python anndata validates that the index of a `data.frame` in `obsm`/
`varm` equals the parent's names and raises while constructing the
AnnData, which made the whole file unreadable:
ValueError: value.index does not match parent's obs names
anndataR itself did not notice because its reader takes the row names of
`obsm`/`varm` entries from `obs_names`/`var_names` rather than from the
stored `_index`, so an R-only roundtrip looked correct.
Pass the parent's names down to `data.frame` elements of the aligned
mappings so the index written to disk is always the one the spec
requires. This also covers `data.frame`s supplied without row names,
which would otherwise be indexed by the same automatic sequence. Entries
that are not data frames are unaffected, as are `uns` data frames, which
keep providing their own index.
|
Thanks for the PR. I would like to look into why this isn't an issue for the existing tests, they should already include data frames in these slots (I think). This has definitely come up in implementation before so I think/hope it shouldn't be a general problem (but maybe I'm wrong). |
|
Bencher Report| Project | anndataR |
| Branch | fix-obsm-varm-data-frame-index |
| Testbed | ubuntu-latest |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🚨 1 Alert
🐰 View full continuous benchmarking report in BencherThe roundtrip tests only ever put matrices and vectors in obsm/varm, so the data frame path was never exercised against Python `anndata`. Python stores those types as arrays, so the elements always came back into R as matrices and neither the data frame read nor the data frame write path was reached. Add the `df_` generator types, which dummy_anndata already supports. It does not name a data frame element after the requested type: every `df_<type>` request is folded into a single element named "dataframe" with the requested types as its columns. Pair each type with the key it is stored under, so that the existing loop can be reused instead of duplicated. The types excluded for dummy-anndata#12 are included as data frames, because they round trip correctly as columns of one in the same way that they do for obs/var. `py_to_r()` keeps the pandas index as the row.names attribute of the converted data frame, and `dimnames(x) <- NULL` is an error for a data frame, so rebuild it before comparing values. Adds 90 tests over 10 column types and three formats. The 30 write tests fail without the preceding fix; the read tests pass either way, as they never write from R.
The h5diff comparison is the only place that generates obsm/varm data in R rather than reading it from a Python file, but it never checked that a data frame there uses the obs_names/var_names as its index. It cannot be checked against the Python file: the two generators use different observation and variable names, `cell1` against `Cell000`, and a data frame index is those names, so the two always differ. Compare the index against the parent names within the R generated file instead, which is the invariant the spec requires.
|
Thank you for making this package! But there are 2 mechanisms where the bug escapes:
This is the py→R→py test and the one you'd expect to catch it — it's the only test that writes an obsm element from R and reads it back with Python. But its input is the Python fixture, not R's generator. dummy_anndata puts vector types in obsm as an (n, 1) ndarray, not a DataFrame. I added a fix (first commit) adding _df datatypes
This is the h5diff test, and the only one that builds its R side with generate_dataset() — so it's the only place a data frame really is written from R. The bad _index is sitting in file_r2. But it compares Python's /obsm/dense_array (array dataset) against R's /obsm/numeric (dataframe group) which gives: h5diff exits 0 for "not comparable", and the only assertion is expect_equal(res_obsm$status, 0), so those four comparisons (character, numeric, integer, logical) pass without comparing anything. Two test commits pushed: df_ types wired into the roundtrip so test 1 actually gets a data frame from Python (30 write tests, all failing without the fix), and an index check in test 2 comparing _index against obs_names/var_names within the R file — it can't be checked across files since the generators use cell1 vs Cell000. The problem that h5diff passes when comparing groups to datasets still exists. Up to you to decide if you want to fix this, since this would be affecting other tests I imagine. |
3 participants
Description
Writing a
data.frameintoobsm/varmproduced an.h5ad/.zarrthat Python anndata cannot open at all throwing aValueErrorwhile constructing theAnnData. This happens because stripping the data.frame index means it gets replaced with Rs default index and that mismatches the .obs index.On disk, before this PR:
Cause
.validate_aligned_array()strips row names before storage withrownames(mat) <- NULL, becauseobsmrow names are redundant withobs_namesand the array encoding has nowhere to put them.data.framealways has row names, sorownames(df) <- NULLdoes not remove them — R silently substitutes the automatic1:nrowsequence.write_h5ad_data_frame()then does the right thing with what it is given and writes those automatic row names as_index. Itsis.null(index_value)fallback never fires, which is why the stored index is 1-based"1".."n"rather than the 0-based0..n-1the fallback would produce.Per the spec,
obsmentries may be dataframes, and Python anndata requires the index of such a dataframe to equal the parent'sobs_names.Fix
Pass the parent's
obs_names/var_namesdown todata.frameelements of the aligned mappings, so the index written to disk is always the one the spec requires:write_h5ad_mapping()/write_zarr_mapping()gain anindexargument, forwarded only to elements that are data frames.obsm/varmsetters ofHDF5AnnDataandZarrAnnDatapassself$obs_names/self$var_names.Taking the index from the parent rather than preserving the row names also covers data frames supplied without row names, which would otherwise be written with the same automatic sequence.
Deliberately left alone:
.validate_aligned_array()still strips row names. For adata.framethe result is automatic row names, whichhas_row_names()already reports as "no row names" — consistent with the documented architecture whereobsm/varmare stored without dimnames and names are re-added on access.unsdata frames keep providing their own index.obs_namesis unchanged, since it matches what anndataR reports asobs_namesfor such objects. (Python uses0..n-1there; that discrepancy predates this PR and is out of scope.)Verification
The reproducer above now round-trips, and the file reads cleanly under Python anndata 0.12.18 and 0.13.2.
Tests added
test-HDF5AnnData.R— asserts the raw on-disk_indexofobsm/varmdata frames equalsobs_names/var_names, for data frames with and without row names, and that matrix entries are unaffected. It has to check the stored bytes: reading back through anndataR would mask the bug.test-HDF5-write.R— unit test for the newindexargument ofwrite_h5ad_mapping().test-roundtrip-obsmvarm-data-frame.R— Python interop regression test overh5ad,zarrv2andzarrv3: R writes → Python reads and the index matches; and Python writes → R reads → R writes → Python reads.