Repository navigation
convert: write SCope looms with --scope (#61) - #70
Conversation
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.
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. |
![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: cb2fb38611
ℹ️ 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".
| elif enc in (spec.STRING_ARRAY, spec.NULLABLE_STRING_ARRAY) or ( | ||
| getattr(member, "dtype", None) is not None and member.dtype.kind in "OSUT" | ||
| ): | ||
| values = natural_sorted(set(read_str_all(member, parent_group=obs))) |
There was a problem hiding this comment.
When --scope is used on an AnnData with a string obs column, read_str_all() materializes every value and set() temporarily duplicates them, even though the exporter discards the annotation after finding 246 distinct values. Large or high-cardinality cell metadata can therefore make planning consume memory proportional to the full column and OOM before conversion; scan chunks and stop once MAX_ANNOTATION_VALUES is exceeded.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ffcea1: _distinct_text reads the column a block at a time and returns as soon as it has more than MAX_ANNOTATION_VALUES, so a barcode-like column costs one block past the limit. Test: test_a_text_column_with_too_many_values_is_no_annotation_and_is_not_read_whole.
| return ScopeExport( | ||
| tables, embeddings, found, metadata, | ||
| cluster_id=bool(found) and "ClusterID" not in obs_columns, regulons=regulons, |
There was a problem hiding this comment.
When the source already has an obs column named ClusterID, this disables generation of SCope's ClusterID attribute and the ordinary-column export preserves the existing values. This is especially problematic after importing a SCope loom and then changing its first clustering: Clusterings["0"] contains the new codes while ClusterID remains stale, producing internally inconsistent output for consumers that use ClusterID; exclude or validate the existing column and derive this attribute from the first exported clustering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ffcea1: with any clustering exported, ClusterID is always written from the first one; an obs column of that name is left out of the plain attributes and of the metrics, with a warning saying so. Test: test_cluster_id_is_written_from_the_first_clustering_not_a_stale_column.
| regulons = key.endswith("Regulons") or key.endswith("RegulonsAUC") | ||
| taken: Set[str] = set() | ||
| fields = [claim(c.replace(" ", "_") if regulons else c, taken) for c in numeric] | ||
| renamed = [f"{c!r} as {f!r}" for c, f in zip(numeric, fields) if c != f] |
There was a problem hiding this comment.
When regulon names collide after spaces are replaced—for example, A B and A_B—claim() assigns suffixes independently for each dataframe. If obsm["RegulonsAUC"] and varm["Regulons"] contain those same regulons in different column orders, the same exported field name can refer to one regulon's AUC and another regulon's membership, silently associating scores with the wrong targets; build and apply a shared original-name-to-field mapping across the matching tables.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ffcea1: one original-name → field map is built across every regulon table (regulon_fields) and used for AUC, Regulons and the thresholds alike. Test: test_regulons_that_meet_once_spaces_go_keep_one_field_in_both_tables (fails before the fix).
cb2fb38 to
9ffcea1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ffcea1197
ℹ️ 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".
| if enc == spec.CATEGORICAL: | ||
| values = [str(c) for c in read_categories(member, parent_group=obs)] |
There was a problem hiding this comment.
When an unselected categorical obs column has more than 245 categories, read_categories() materializes every category label before the exporter discards the annotation. Explicit categoricals such as barcode columns can therefore make planning consume memory proportional to input cardinality and OOM; inspect the categories array length first and only read it when it is within MAX_ANNOTATION_VALUES.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab5156: a categorical whose categories array is longer than MAX_ANNOTATION_VALUES is skipped on its length, before read_categories. Test: test_a_categorical_with_too_many_categories_is_no_annotation_and_its_categories_are_not_read.
| new = renumbered.get(old) | ||
| if new is None: | ||
| warnings.append(f"Not written: ClusterMarkers_{old} -- no clustering written has id {old}.") | ||
| continue |
There was a problem hiding this comment.
For a plain AnnData using --clusterings, every selected column has old=None, so renumbered remains empty and even a matching varm["ClusterMarkers_0_avg_logFC"] is discarded here. This prevents the documented marker-table export unless the input was previously imported from SCope; newly selected clusterings need an identity mapping from their assigned position so matching marker tables are retained.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab5156: a clustering no import recorded owns ClusterMarkers_<its new id> (unless a recorded clustering already had that id). Test: test_a_clustering_named_with_clusterings_keeps_its_marker_tables.
| for table in group_tables: | ||
| accessor = _MARKERS.match(table.key).group(2) | ||
| table.key = f"ClusterMarkers_{new}" + (f"_{accessor}" if accessor else "") | ||
| keep.append(table) |
There was a problem hiding this comment.
When a recorded clustering uses non-contiguous cluster IDs, such as 2 and 5, export renumbers the metadata clusters to 0 and 1 but only renames the ClusterMarkers_<clustering> dataset here; its compound fields remain 2 and 5. SCope then cannot associate those marker columns with the newly numbered clusters, so the marker field names must be remapped from the recorded cluster IDs to the new category positions as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dab5156: for a recorded clustering, each recorded cluster id is mapped (through its description, as the import labelled the categories) to its category's position, and the marker tables' fields are renamed with it; a field for a cluster no longer present is left out with a warning. Test: test_marker_fields_are_renumbered_with_their_clusters (ids 2, 5, 9 → 0, 1, 2).
A loom export wrote a plain loom, so results edited as AnnData could not go back to the SCope viewer. With --scope: - obsm and varm dataframes (RegulonsAUC, Regulons, ClusterMarkers_<id>) become compound attributes of their numeric columns, a block of rows at a time; regulon names lose their spaces, alike in AUC and Regulons, as SCope needs; - two-column obsm arrays become Embeddings_X/_Y (and Embedding for the default, id -1), not again plain attributes; - the categorical columns a SCope import recorded, and --clusterings, become Clusterings and ClusterID, numbered 0..n-1 with their clusters described, as SCope's server indexes them; - MetaData (plain JSON): embeddings, clusterings (markers only with their metrics, else left out: SCope crashes on them), metrics, annotations, regulon thresholds; recorded ids and names are kept, so a SCope loom round-trips through h5ad. A non-categorical clustering, an unknown --default-embedding, or a table too wide for one HDF5 compound type (64 KiB) is refused before writing. --clusterings and --default-embedding need --scope; a plain export of a SCope-born AnnData says --scope would keep its regulons. Checked on the Zenodo pySCENIC loom: loom -> h5ad -> --scope loom equals the original through the pySCENIC recipe, and meets SCope's reading rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, bounded annotation scan - Regulon names that meet once spaces go (`A B`, `A_B`) were given suffixes table by table, so a field could pair one regulon's AUC with another's targets; one map now names them in every table and in the thresholds. - An obs column named ClusterID was written as it was in place of SCope's own; ClusterID now always comes from the first clustering. - A text column's distinct values are read a block at a time and the scan stops once there are more than an annotation holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9ffcea1 to
fe33021
Compare
…s, bound category scan - Marker tables' fields are cluster ids: renumbered with the clusters, so recorded ids 2, 5, 9 become 0, 1, 2 there as in MetaData. - A clustering named with --clusterings owns ClusterMarkers_<its id>; only recorded clusterings' tables were kept. - A categorical with more categories than an annotation lists is skipped by its length, without reading the categories. 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: dab5156919
ℹ️ 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".
| "as one compound attribute (its type must fit 64 KiB); pySCENIC's own " | ||
| "export stops at the same limit. Keep fewer columns first." | ||
| ) | ||
| tables.append(ExportTable(axis, key, f"{where}/{key}", numeric, fields, dtype)) |
There was a problem hiding this comment.
When --scope is used on a valid AnnData where an obsm dataframe shares a key with an obs column, such as scores, this unconditionally plans both under the same flat col_attrs/scores name. The writer creates the ordinary attribute first and then _write_records attempts to create the table at the existing name, so conversion fails only after writing has begun; tables colliding with reserved names such as CellID or Embeddings_X fail similarly. Detect these collisions in the plan and either refuse them or apply an explicit precedence rule.
AGENTS.md reference: AGENTS.md:L127-L128
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24a1b36, for plain loom exports as well (they had the same collision): _one_owner_per_name runs in the plan and gives each col_attrs/row_attrs name to the first that asks — CellID/Gene/Accession and SCope's own attributes, then obs/var columns, then SCope's tables, then obsm/varm arrays — leaving the others out with a warning, before the output exists. Metrics and annotations follow the columns kept. Test: test_two_elements_asking_for_one_attribute_name_keep_the_first_and_warn.
| cluster_fields[position] = { | ||
| _label(number): str(position_of[label]) | ||
| for (number, _), label in zip(clusters, labels) if label in position_of | ||
| } |
There was a problem hiding this comment.
When importing a SCope loom whose metadata omits a clustering's clusters list, the supported import path infers category labels from the Clusterings values and records the clustering ID in uns["scope"]. On re-export, old is therefore set but clusters is empty, so this branch installs an empty field map; the marker loop then classifies every field of a matching ClusterMarkers_<id>_<metric> table as unknown and drops the table. Fall back to mapping the inferred category labels to their positions when no recorded cluster descriptions exist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24a1b36: with no recorded clusters the import labels categories by cluster id, so the field map is now those labels to their positions. Test: test_markers_of_undescribed_clusters_are_kept_on_a_round_trip.
… clusters - An obs column and an obsm entry (or a SCope part, or Gene) of one name were both written to col_attrs/row_attrs, and the second write failed with the output half written. The plan now gives each name to the first that asks and leaves the rest out with a warning. - A clustering whose clusters MetaData did not describe is labelled by id on import; on export its marker fields now map through those labels instead of being dropped. 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: 24a1b36299
ℹ️ 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".
| if not thresholds and "uns" in root and "scope" in root["uns"] and "regulon_thresholds" in root["uns"]["scope"]: | ||
| for regulon, row in _frame_rows(root["uns"]["scope"]["regulon_thresholds"]).items(): |
There was a problem hiding this comment.
When an AnnData was imported from SCope and its uns["scope"]["regulon_thresholds"] dataframe is subsequently edited or extended, the original JSON still contributes at least one entry here, so the dataframe fallback never runs and the export silently restores the stale thresholds. Treat the structured dataframe as the current source whenever it exists, rather than consulting it only when the cached metadata yields no thresholds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f29dc92: when uns["scope"]["regulon_thresholds"] exists its rows and values win; the recorded MetaData only adds what the table does not hold (allThresholds). Test: test_an_edited_thresholds_table_wins_over_the_recorded_metadata.
| if recorded.get("clusterMarkerMetrics"): | ||
| entry["clusterMarkerMetrics"] = recorded["clusterMarkerMetrics"] |
There was a problem hiding this comment.
When a SCope-imported AnnData has one or all recorded marker-metric dataframes removed or replaced, this copies the old clusterMarkerMetrics list before checking which ClusterMarkers_<id>_<accessor> tables still exist. The resulting MetaData can advertise absent accessors or omit newly added ones, leaving the viewer metadata inconsistent with the exported row attributes; prune or derive the list from group_tables instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f29dc92: clusterMarkerMetrics is now built from the ClusterMarkers_<id>_<accessor> tables being written — recorded entries kept (with their names) only for accessors still present, new tables appended under their accessor — and is not copied otherwise. Test: test_marker_metrics_list_the_tables_written.
…from the tables written 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: f29dc92c14
ℹ️ 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".
| found = scope.plan_export( | ||
| root, obs_columns=[c for c in obs_columns if c != "CellID"], | ||
| varm_key=varm_key, clusterings=options.clusterings, | ||
| default_embedding=options.default_embedding, warnings=warnings, |
There was a problem hiding this comment.
When any recorded or explicitly selected clustering is exported, this passes its obs column into the ordinary-column plan as well as into Clusterings. Re-importing the resulting loom sees the ordinary column first, so _clusterings() must rename the compound copy (for example, leiden becomes leiden-1); repeated round trips accumulate duplicate columns and move the recorded clustering key each time. Remove exported clustering columns from obs_columns and from annotation metadata, just as embedded obsm keys are removed below.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f4b477: clustering columns are removed from the plain obs attributes and from the annotations, so a clustering goes out once, as Clusterings, and comes back under its own name. Test: test_a_clustering_is_written_once_and_comes_back_under_its_own_name.
| if found is not None: | ||
| found.tables = [ | ||
| t for t in found.tables | ||
| if claim_name("col_attrs" if t.axis == "col" else "row_attrs", t.key, t.source) | ||
| ] |
There was a problem hiding this comment.
When a var column and a marker dataframe request the same row-attribute name, this post-planning filter drops the marker table but leaves the already-built clusterMarkerMetrics entry in found.metadata. The emitted SCope metadata then advertises an accessor whose compound dataset is absent (or replaced by the ordinary 1-D column), recreating the metadata/table inconsistency. Fresh evidence beyond the earlier marker-metadata report is this later namespace-collision filter; prune or rebuild marker metadata after it runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f4b477: scope.prune_markers runs after the name filter; each clustering's clusterMarkerMetrics keeps only accessors whose table is still written, and without its ClusterMarkers_<id> table or any metric the clustering's markers are left out whole, with a warning. Test: test_markers_whose_table_lost_its_name_are_left_out_with_their_metrics.
| if spec.encoding_type(obs[column]) != spec.CATEGORICAL: | ||
| raise ValueError( | ||
| f"--clusterings {column}: not a categorical obs column, which SCope's " | ||
| "clusterings need (cluster ids with descriptions)." |
There was a problem hiding this comment.
For anndata 0.7-era files, a categorical obs column is an untagged integer dataset whose categories attribute points into __categories, so encoding_type() returns None and this rejects a valid --clusterings selection. Detect the legacy categorical layouts already supported by read_categories() rather than requiring only the modern group encoding; otherwise older supported h5ad inputs cannot use the new SCope export path.
AGENTS.md reference: AGENTS.md:L71-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f4b477: scope.is_categorical accepts anndata 0.7's codes-with-categories-reference layout, and categorical_codes reads its codes when writing Clusterings; such columns also count as annotations rather than metrics. Test: test_an_anndata_07_categorical_can_be_a_clustering.
… anndata 0.7 categoricals - A clustering went out as Clusterings and as a plain attribute; read back, the copy took its name and the clustering became leiden-1. - Marker metrics are pruned again once tables lose a name to another attribute, and markers without their base table or any metric are left out whole. - --clusterings takes anndata 0.7's categoricals (codes with a categories reference), and they are annotations, not metrics. 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: 3f4b477fc3
ℹ️ 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".
| if is_group(member) and len(member["categories"]) > MAX_ANNOTATION_VALUES: | ||
| continue # too many to list, and not read to find that out | ||
| values = [str(c) for c in read_categories(member, parent_group=obs)] |
There was a problem hiding this comment.
When an unselected anndata 0.7 categorical has a large referenced category dataset, this guard is skipped because the legacy column is a dataset rather than a group, and the following read_categories() call materializes every label before discarding the annotation past 245 values. Fresh evidence beyond the earlier categorical report is the newly added is_categorical() support for this legacy dataset layout; resolve the referenced category dataset's length and skip it before reading, as is done for modern categoricals.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b68045a: elements.read.categories_dataset resolves where a categorical's labels live (group, 0.7 HDF5 reference, __categories, legacy Zarr path) without reading them, read_categories now uses it, and the annotation scan checks its length first for every layout. Test: test_an_anndata_07_categorical_with_too_many_categories_is_not_read.
| if key == "defaultThresholdValue": | ||
| entry[key] = _float(value) |
There was a problem hiding this comment.
When uns["scope"]["regulon_thresholds"] contains a missing or non-numeric defaultThresholdValue—including a round trip where only some imported threshold entries define that field—_float() returns NaN here. The later json.dumps() writes the non-standard token NaN, so strict JSON readers such as the SCope client cannot parse the entire MetaData attribute; omit the field, encode it as null, or refuse the invalid value instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b68045a: a non-finite defaultThresholdValue from the table is left out of the entry, and the whole MetaData goes through scope.json_safe (NaN/inf → null) before json.dumps. Test: test_a_missing_threshold_is_left_out_of_strict_json parses it with a parse_constant that refuses NaN.
| if found is not None: | ||
| scope.prune_markers(found, warnings) | ||
| for part in ("metrics", "annotations"): | ||
| found.metadata[part] = [m for m in found.metadata[part] if m["name"] in obs_columns] |
There was a problem hiding this comment.
When an obs column and an obsm regulon dataframe both request a name such as RegulonsAUC, _one_owner_per_name() removes the compound AUC table after plan_export() has already built regulonThresholds and the regulon count from it. This post-filter cleanup only repairs markers and obs metadata, so the output can retain thresholds for a compound AUC attribute that was dropped and replaced by an unrelated one-dimensional column; recompute or prune the regulon metadata from the surviving tables here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b68045a, by moving name ownership ahead of the plan instead of filtering after it: _column_owners settles loom's/SCope's own attributes and the columns, plan_export(owners=...) drops a table that loses its name before thresholds, the regulon count or marker metrics are derived (marker tables claim theirs once renumbered), and arrays claim last. That replaces the post-filter prune_markers. Test: test_an_auc_table_that_loses_its_name_takes_its_regulon_metadata_with_it.
… bounded legacy categories - Names are given out before the plan derives anything from the tables: a table that loses its name (an obs column RegulonsAUC) is gone before thresholds, the regulon count or marker metrics are worked out from it, so MetaData only describes tables written. Marker tables claim their names once renumbered. - NaN and infinities in MetaData become null, and a missing defaultThresholdValue is left out: SCope parses strict JSON. - A legacy (anndata 0.7) categorical's categories are counted by the length of the dataset they point to, before any is read (elements.read.categories_dataset). 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: b68045af32
ℹ️ 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".
| labels = read_str_all(index) | ||
| rows: Dict[str, Dict[str, str]] = {label: {} for label in labels} | ||
| for column in dataframe_columns(group, name): | ||
| for label, value in zip(labels, read_str_all(group[column], parent_group=group)): | ||
| rows[label][column] = value |
There was a problem hiding this comment.
When uns/scope/regulon_thresholds contains many rows, including thresholds unrelated to the exported AUC fields, _frame_rows() loads the complete index and every column and then duplicates them into per-row dictionaries before the caller filters against auc_fields. Planning can therefore consume memory proportional to the entire table and OOM before conversion; scan rows in bounded chunks and retain only exported regulons.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this one, deliberately. The thresholds table is one row per regulon, written by the import from MetaData's regulonThresholds — and the export reads that same MetaData JSON whole (_recorded), as it must to keep its other keys, so the table never sets peak memory. Its size is also bounded in practice by the regulons a SCope AUC table can hold: one compound type of at most 64 KiB, a few thousand fields. Streaming it would not change the bound.
| table.fields = [f for f, _, _ in kept] | ||
| table.columns = [c for _, c, _ in kept] | ||
| table.dtype = np.dtype([(f, t) for f, _, t in kept]) | ||
| if not table.fields: | ||
| continue |
There was a problem hiding this comment.
When an imported clustering is edited so that a marker table's old cluster IDs no longer map to any current category, this branch drops the now-empty table after clusterMarkerMetrics has already been assigned. The emitted MetaData consequently advertises an accessor whose ClusterMarkers_<id>_<accessor> dataset is absent; filter the tables first and derive the metrics only from those that still have fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 78287f0: the fields are renumbered and empty tables dropped first; clusterMarkerMetrics is then derived only from the tables still written (and markers without their base table are left out whole). Test: test_a_marker_table_left_with_no_cluster_is_not_listed_as_a_metric.
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: 78287f0584
ℹ️ 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".
| clustered = {c.column for c in found.clusterings} | ||
| obs_columns = [c for c in obs_columns if c not in clustered] |
There was a problem hiding this comment.
When a selected clustering and an obsm entry share a key, _column_owners() first assigns col_attrs/<key> to the obs column, and plan_export() or the later array filter consequently rejects the obsm entry. This line then removes the obs column because the clustering is actually written only inside Clusterings, leaving the claimed attribute name unused and silently losing the otherwise exportable obsm data. Exclude clustering columns before ownership is resolved, or release and reconsider their claims before filtering tables and arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ef1d08: scope.clustering_columns names the clustering columns up front (recorded, then --clusterings), and they are taken out before _column_owners, so they never claim a name; an obsm entry of the same name is written. Test: test_a_clustering_leaves_its_name_to_an_obsm_entry.
| metrics = [m for m in recorded_metrics.get(new, []) if m["accessor"] in accessors] | ||
| listed = {m["accessor"] for m in metrics} | ||
| metrics += [{"accessor": a, "name": a, "description": ""} for a in accessors if a not in listed] | ||
| if not metrics or not any(t.key == base for t in group_tables) and any(t.key == base for t in named): |
There was a problem hiding this comment.
When the input contains ClusterMarkers_0_avg_logFC but no ClusterMarkers_0, the final any(... in named) is false, so this guard allows the metric table through and adds clusterMarkerMetrics even though the base marker dataset is absent. The earlier collision case is fixed, but this fresh no-base-from-the-start path still produces metadata and tables that SCope cannot use; require base to remain in group_tables unconditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ef1d08: a clustering's markers are written only when ClusterMarkers_<id> itself is among the tables written, whatever the reason it is not. Test: test_metric_tables_without_their_markers_table_are_left_out.
…eir base table 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: 7ef1d08f82
ℹ️ 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".
| if numeric and (_MARKERS.match(key) or claim_attr( | ||
| owners, "col_attrs" if axis == "col" else "row_attrs", key, f"{where}/{key}", warnings | ||
| )): |
There was a problem hiding this comment.
When an obsm dataframe named ClusterMarkers_0 shares its key with an obs column, this marker-name exemption skips claim_attr, although only row-axis marker tables are handled by the later marker logic. The obs column and compound table are therefore both planned for col_attrs/ClusterMarkers_0, and _export_scope fails on the duplicate dataset after writing has begun. The fresh evidence beyond the earlier collision report is this _MARKERS bypass for column-axis frames; restrict it to row-axis marker tables or claim column-axis names normally.
AGENTS.md reference: AGENTS.md:L127-L128
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a733bef: the marker exemption is now for row-axis (varm) tables only; an obsm dataframe named ClusterMarkers_* claims its name like any other table. Test: test_an_obsm_table_named_like_markers_claims_its_name.
| if found.embeddings: | ||
| default = found.embeddings[0] | ||
| for axis, attr in ((0, "Embeddings_X"), (1, "Embeddings_Y")): | ||
| dtype = np.dtype([(str(e.id), np.dtype(obsm[e.key].dtype.str)) for e in found.embeddings]) |
There was a problem hiding this comment.
With sufficiently many two-column obsm arrays, the generated Embeddings_X/Embeddings_Y compound dtype exceeds HDF5's 64 KiB type-message limit. Unlike dataframe compound dtypes, which are checked with compound_fits() during planning, this dtype is first given to h5py after the matrix and ordinary attributes have already been written, so conversion performs potentially substantial work before failing. Build and validate the generated embedding dtype in the plan as well.
AGENTS.md reference: AGENTS.md:L127-L128
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a733bef: the plan builds the Embeddings_X/_Y and Clusterings compound types and refuses one HDF5 cannot hold (_check_fits, the same compound_fits probe as the tables), before the output exists. Test: test_too_many_embeddings_for_hdf5_are_refused_before_writing.
…rings checked against HDF5's limit in the plan 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: a733beff93
ℹ️ 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".
| owners["col_attrs"].update({ | ||
| scope.EMBEDDING: "SCope's default embedding", scope.EMBEDDINGS_X: "SCope's embeddings", | ||
| scope.EMBEDDINGS_Y: "SCope's embeddings", scope.CLUSTERINGS: "SCope's clusterings", | ||
| }) |
There was a problem hiding this comment.
When --scope is used without any two-column embedding candidates, an ordinary obs column named Embedding is removed here because the name is preassigned to SCope, but _export_scope() never creates that attribute. The same data loss occurs for the other fixed names when their corresponding SCope part is absent; reserve each name only after the plan determines that it will actually emit that dataset.
Useful? React with 👍 / 👎.
| # Clusterings: the recorded ones, and those --clusterings names. | ||
| obs = root["obs"] | ||
| wanted = clustering_columns(root, clusterings) | ||
| entries = {str(e.get("id")): e for e in parsed.get("clusterings", []) if isinstance(e, dict)} |
There was a problem hiding this comment.
If an imported loom has valid JSON such as "clusterings": null (or another non-list value), the import path tolerates it via _by_id(), but a subsequent --scope export tries to iterate that value here and fails during planning. Normalize this field to an empty list unless it is actually a list, matching the import behavior.
Useful? React with 👍 / 👎.
Closes #61. Stacked on #69; it will retarget to
mainonce that merges.adata convert x.h5ad -o x.loom --scopewrites a loom the SCope viewer reads, so results edited as AnnData can go back to SCope, and a SCope loom round-trips through h5ad.RegulonsAUC,Regulons,ClusterMarkers_<id>[_<metric>]compound attributesEmbedding(the default) andEmbeddings_X/_Yby id--clusterings a,bClusterings(int codes, -1 unassigned) andClusterIDannotations(categorical/text) andmetrics(numeric)uns["scope"]regulonThresholdskept--default-embedding KEYpicks the embedding SCope shows first (else the recorded default, thenX_umap/X_tsne, then the first)._(with a warning); clusterings and clusters are renumbered0..n-1, as SCope indexes them by position; cluster markers without metrics are left out with a warning, since SCope crashes on them.--clusteringscolumn that is not categorical, a--default-embeddingthat is not a two-column obsm array, a table wider than one HDF5 compound type holds (the 64 KiB limit, pySCENIC #339), and either flag without--scope.--scopewould keep its regulons.Verified: the Zenodo pySCENIC pbmc3k loom → h5ad →
--scopeloom reads, through the pySCENIC tutorial's loompy recipe, the same AUC, regulons, embeddings, clusterings and thresholds as the original, and passes a re-implementation of SCope's server-side reading rules. SCope's server itself is not on PyPI, so it was not run. An integration test runs the loompy recipe on an export; a perf guard counts reads and writes.🤖 Generated with Claude Code