Repository navigation
concat: stack obsm dataframes, merge varm - #69
Conversation
SCope and pycisTopic imports leave dataframes in obsm (RegulonsAUC) and
varm (Regulons, topic_region). concat skipped the obsm ones ("not a dense
array in every input") and never wrote varm at all, though its docstring
said the alternative axis follows --merge, as anndata's concat does.
- obsm: a dataframe in every input is stacked as obs is, its columns
following --join, through the obs column writers.
- varm: merged as var's columns are, on the output's genes. same/unique
compare a snapshot read once per input (exact bytes, so NaN equals NaN);
first/only go by presence. An input lacking some genes (an outer join)
counts as missing. Without --merge it is dropped, with a note.
Checked against anndata.concat for each join and strategy, and on two
h5ads converted from the Zenodo pySCENIC loom.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Test Results (py3.12)1 681 tests 1 681 ✅ 4m 17s ⏱️ Results for commit 800c409. ♻️ This comment has been updated with latest results. |
Test Results (py3.13)1 681 tests 1 681 ✅ 2m 20s ⏱️ Results for commit 800c409. ♻️ This comment has been updated with latest results. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a671d14f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """A comparable value of a varm entry, on the target var: its bytes for | ||
| an array; each column's, for a dataframe. Exact, so NaN equals NaN.""" | ||
| if is_dataset(obj): | ||
| values = np.asarray(obj[...])[where] |
There was a problem hiding this comment.
When --merge same or unique encounters a large varm array or dataframe column, this reads the entire source into memory, copies the aligned selection, and retains a full byte snapshot for every input; the later write also materializes the whole selected entry. Consequently, peak memory scales with the complete varm data and input count rather than --chunk, so realistic pycisTopic region-by-topic arrays can exhaust memory during an otherwise streamed concat. Compare and copy these entries in chunks instead.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 65060c6. Under same/unique each input's entry is now reduced to a blake2b digest read --chunk genes at a time (_varm_snapshot), and the kept entry is copied the same way (_write_varm_array). Genes in another order are read sorted and put back in the target's order per block. A dataframe entry is digested one column at a time, each column var-long, the same budget _write_var uses for var's columns.
| if _concat_obs_column(frame, column, [s[column] if column in s else None for s in sources], | ||
| lengths, console) |
There was a problem hiding this comment.
For an obsm dataframe with many observations, this delegates each column to _concat_obs_column, whose numeric, masked, categorical, and string paths read complete source columns and allocate a complete output column. Thus adding RegulonsAUC support makes concat memory grow with total observations regardless of chunk_rows; these columns need a chunked writer rather than reusing the in-memory obs-column implementation.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is, deliberately: an obsm dataframe is written one column at a time, and each column is n_obs long — the same budget every obs column already takes in _write_obs through the same _concat_obs_column, and smaller than the obs index (target_obs) concat holds whole. So it doesn't move peak memory past what obs alone sets. Streaming obs-length columns in blocks would be a change to obs as much as to obsm; I'd rather do that for both in its own PR.
| an array; each column's, for a dataframe. Exact, so NaN equals NaN.""" | ||
| if is_dataset(obj): | ||
| values = np.asarray(obj[...])[where] | ||
| return ("array", str(values.dtype), values.shape, values.tobytes()) |
There was a problem hiding this comment.
When a dense varm array contains HDF5 variable-length strings, np.asarray produces an object-dtype array and tobytes() serializes Python object pointers rather than string contents. Independently read arrays with identical strings can therefore compare unequal—or accidentally compare equal after address reuse—causing same/unique to keep or drop the key incorrectly; use semantic value comparison as anndata's merge implementation does.
AGENTS.md reference: AGENTS.md:L145-L146
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 65060c6: text is now fed to the digest as its UTF-8 text, length-prefixed (_digest), never as object bytes. Test: test_varm_text_arrays_are_compared_by_their_text.
| source = next(i for i, v in enumerate(aligned) if v is not _MISSING) | ||
| obj, take = roots[source]["varm"][name], positions[source] | ||
| if is_dataset(obj): | ||
| write_dense(varm, name, np.asarray(obj[...])[take]) |
There was a problem hiding this comment.
When a kept dense varm entry comes from a chunked or compressed HDF5/Zarr dataset, write_dense recreates it without the source's dataset creation settings, silently discarding its chunking and compression and potentially inflating the output substantially. Derive and clamp the creation kwargs from the selected source before writing.
AGENTS.md reference: AGENTS.md:L134-L135
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 65060c6: the output dataset now takes dataset_create_kwargs from the chosen source, fitted with fit_chunks, as subset's copies do. Test: test_a_kept_varm_array_keeps_its_compression.
Comparing varm under same/unique read every input's whole entry and kept its bytes; now each input is reduced to a digest a block of rows at a time, and the kept entry is copied the same way, with the source's chunking and compression. Text arrays are compared by their text, not by the object pointers their bytes held. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65060c6901
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| kind = "text" if is_string_dtype(obj.dtype) else str(obj.dtype) | ||
| return ("array", kind, tuple(obj.shape[1:]), hasher.digest()) |
There was a problem hiding this comment.
When corresponding varm arrays store equal values in different numeric dtypes—for example, int32 versus int64—including str(obj.dtype) makes their snapshots distinct even though anndata's array equality considers their values equal. Consequently, --merge same and --merge unique silently drop a key that the documented oracle retains; normalize numeric values for comparison rather than including the storage dtype in their identity.
AGENTS.md reference: AGENTS.md:L145-L146
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked against the oracle, and anndata does the opposite: with anndata 0.13.3, ad.concat(..., merge="same") drops a varm array that is int64 in one input and int32, float32 or float64 in the other, and keeps it only when the dtypes match too. So keeping the dtype in the comparison is what matches it. Pinned in 800c409: test_varm_in_another_dtype_merges_as_anndata_merges_it compares our keys with anndata.concat's for each dtype.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SCope and pycisTopic imports put dataframes in obsm (
RegulonsAUC) and varm (Regulons,topic_region).concatskipped the obsm ones ("not a dense array in every input") and never wrote varm at all, though its docstring said the other axis follows--merge.--join innerkeeps the columns every input has;--join outertakes their union, missing where an input lacks one. An array in one input and a dataframe in another is still skipped, with a note.--mergeas var's columns do (same,unique,first,only). Each input's entry is aligned on the target var; an input without some of the target's genes (outer join) counts as missing. Arrays and dataframes both. Without--merge, varm is dropped with a note.Tests compare against
anndata.concatfor both joins and every merge strategy; two perf guards count reads (varm merge linear in n_var, obsm frames linear in n_obs). Also checked on two h5ads converted from the public pySCENIC pbmc3k loom: obsm and varm equalanndata.concat(..., merge="same").🤖 Generated with Claude Code