fix(concat_on_disk): retain aligned-mapping keys under join="outer" - #2558
Open
alejandro-publius wants to merge 5 commits into
Open
fix(concat_on_disk): retain aligned-mapping keys under join="outer"#2558alejandro-publius wants to merge 5 commits into
alejandro-publius wants to merge 5 commits into
Conversation
`concat_on_disk` concatenated the aligned mappings (`.obsm`/`.varm` and `.layers`) over the intersection of their keys regardless of `join`. Under `join="outer"` this silently dropped any key present in only some inputs (and, when retained keys had differing shapes, raised in the dense path), diverging from in-memory `anndata.concat`, which keeps the union and fills the gaps. The concatenation-axis mapping writer now mirrors `outer_concat_aligned_mapping`: it takes the union of keys for outer joins, generates outer reindexers to pad each key's free axis, and fills keys missing from some objects with `fill_value` (promoting integer dtypes where needed) by materializing just those keys through the same `concat_arrays` path the in-memory concat uses. Keys present in every object that need no reindexing keep the memory-efficient streaming path. Closes scverse#2394
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2558 +/- ##
==========================================
- Coverage 87.81% 85.86% -1.96%
==========================================
Files 49 49
Lines 7799 7833 +34
==========================================
- Hits 6849 6726 -123
- Misses 950 1107 +157
|
…paths Use numeric dense/sparse/df obsm with an unshared key and differing widths, so the test exercises union-of-keys, outer reindexing, and the fill paths for all three element types while staying writable on the minimum supported h5py (the previous fixture had a NaN-filled string column that older h5py cannot write).
alejandro-publius
force-pushed
the
fix-concat-on-disk-outer-mappings
branch
from
July 15, 2026 09:35
530db38 to
04aa7a8
Compare
ilan-gold
reviewed
Jul 15, 2026
| if reindexers is None: | ||
| reindexers = gen_outer_reindexers(elems, ns, new_index=index, axis=axis) | ||
| arrays = [ | ||
| (elem if isinstance(elem, pd.DataFrame) else to_memory(elem)) |
Contributor
There was a problem hiding this comment.
Why do these need to be brought into memory?
Contributor
There was a problem hiding this comment.
Is it to avoid rewriting _write_concat_sequence to handle missing elements?
…ng into memory Addresses review feedback on scverse#2558. The previous approach routed every outer-join `.obsm`/`.varm`/`.layers` key that needed filling through `to_memory()` + in-memory `concat_arrays`, pulling the present objects' full arrays into RAM and defeating the point of `concat_on_disk`. Instead, keep the existing streaming writers and teach them about missing keys directly: `write_concat_dense` contributes a lazy `da.full` block and `write_concat_sparse` contributes an empty sparse block (via `_sparse_fill_block`) for each object missing the key, while present objects stay backed/streamed exactly as they do for `X`. `_write_concat_mappings` now streams every key through `_write_concat_sequence`, so `_concat_missing_mapping_key` is gone. Fill shapes come from `ns` (per-object size along the concat axis) and `_target_free_width` (the reindexed width). Result is byte-identical to `anndata.concat` (regression tests unchanged and still pass) but no longer materializes present arrays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPXJeYVpsPJtcG92Zrc44B
…ls for missing keys Follow-up to the streaming rewrite. Adds two outer-join regression tests: - a non-default `fill_value` with keys missing from the leading object, which exercises the materialized (non-empty) sparse fill and the fill built for the first object. - a sparse `.layers` key missing under `axis=1`, which drives the fill along the layer's column axis. Marks the width-inference guard in `_target_free_width` as unreachable (`# pragma: no cover`): a key only reaches it via the union/intersection of the mappings, so at least one object always carries it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPXJeYVpsPJtcG92Zrc44B
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
concat_on_diskwithjoin='outer'doesn't retain all.obsmfields #2394Problem
concat_on_diskwrites the aligned mappings (.obsm/.varmand.layers) over the intersection of their keys, regardless ofjoin. Underjoin="outer"this silently drops any mapping key present in only some of the inputs — unlike in-memoryanndata.concat, which keeps the union and fills the missing entries. When the retained keys additionally have differing shapes, the dense path raisesValueError: Shapes do not aligninstead of padding.Fix
_write_concat_mappingsnow mirrors the in-memoryouter_concat_aligned_mapping:join="outer"(intersection for"inner", unchanged);fill_value(promoting integer dtypes to float where the fill isNaN), by materializing just those keys through the sameconcat_arrayspath the in-memory concat uses.Keys present in every object that need no reindexing keep the existing memory-efficient streaming path, so the common case is unchanged. As a side effect this also fixes the inner-join case where differing
.obsmwidths previously raised.Tests
test_concat_on_disk_outer_mapping_missing_keys—.obsmwith dense / sparse / dataframe values, one key missing from an object and shared keys of differing widths.test_concat_on_disk_outer_layers_missing_keys— a.layerskey missing from one object with a differingvaraxis (exercises the reindexed path).Both assert the on-disk result equals in-memory
concat. The fulltests/test_concatenate_disk.pysuite passes (498 tests).