Repository navigation
matching and presence - #100
Conversation
stanfish06
commented
Aug 10, 2026
stanfish06
commented
- image based cross matching initial
- depbot
- cross match wire in
- cross matching and h5ad saving update
- fix io
- update: equilvalence test apis to prepare for homotopy computation
- update: homotopy coherence prototype
- update: homotopy coherence
- coherence
- update: coherence
- update: coherence
- update: more stable loop reconstruction and matching
- logging
- many chages: boundary check, cross match
- update: coherence computation
- small fix
- update: coherence
- update: coherence api
- coherence
- presence test
- wire wilcoxon test
- wilcoxon guards
- small fixes
![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: 3e1e6032cc
ℹ️ 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".
| geometric_distance=geo_dist, | ||
| neighbor_rank=rank, | ||
| image_death_simplex=[ | ||
| int(v) for v in image_death_simplices[i] if v >= 0 | ||
| ] | ||
| or None, | ||
| image_death_simplex=image_death_simplex, | ||
| boundary_checked=bool(boundary_checked[i]), | ||
| ) |
There was a problem hiding this comment.
When saving bootstrap results after homological-equivalence checks, _serialize_loop_matches never writes topological_equivalence, and this constructor consequently restores every match with it set to None. Because LoopTrack.filter_matches(keep="equivalent") treats None as accepted, candidates that explicitly failed the boundary check become valid matches after a save/load round trip, changing presence counts and downstream Hodge analyses; the equivalence certificate and coherence data need to be serialized and restored.
Useful? React with 👍 / 👎.
| if CROSS_MATCH_RESULT_KEY in adata.uns and SCLOOP_UNS_KEY not in adata.uns: | ||
| raise ValueError( | ||
| f"adata.uns holds cross-dataset matching results " | ||
| f"('{CROSS_MATCH_RESULT_KEY}') but no '{SCLOOP_UNS_KEY}'. " | ||
| "Use scloop.io.save_cross_match(matcher, path) instead." |
There was a problem hiding this comment.
For every AnnData returned by match_loops, scloop_cross_match is present while scloop is absent, so save_scloop always enters this branch and directs the caller to scloop.io.save_cross_match. No such function exists anywhere in the repository or is exported from this module, leaving expensive cross-dataset matching results with no advertised way to save them.
Useful? React with 👍 / 👎.
| if hd.boundary_matrix_d1.shape[0] > max_rows_boundary_matrix: | ||
| logger.warning( | ||
| f"Boundary matrix has more than {max_columns_boundary_matrix} columns. Downstream computation could be slow" | ||
| f"Boundary matrix has more than {max_rows_boundary_matrix} rows. Downstream computation could be slow" |
There was a problem hiding this comment.
For dense radius graphs, the number of triangle columns grows much faster than the number of edge rows, and this commit also changes Ripser to emit every triangle. Checking shape[0] means, for example, a 200-vertex clique has only 19,900 rows and bypasses the default 30,000 limit despite producing about 1.3 million columns, so autoshrinking never runs and the full matrix proceeds into storage and Hodge computations with severe memory/runtime impact. The guard and loop need to constrain the triangle-column dimension (or both dimensions), not only rows.
Useful? React with 👍 / 👎.
| companion = ad.AnnData( | ||
| X=X, | ||
| obs=adata.obs.copy(), | ||
| var=adata.var.copy(), | ||
| obsm=dict(adata.obsm), | ||
| varm=dict(adata.varm), | ||
| obsp=dict(adata.obsp), | ||
| varp=dict(adata.varp), | ||
| layers=layers, | ||
| uns=uns, |
There was a problem hiding this comment.
When minify=False, the companion is labeled as a full save but this reconstruction omits adata.raw. prepare_adata explicitly populates adata.raw before HVG selection, so the normal save/load path silently discards the original expression matrix and its full gene metadata even though only the scloop-specific uns entries are meant to be stripped; copy raw into the companion for non-minified saves.
Useful? React with 👍 / 👎.
| loop_class.death | ||
| for side in sides | ||
| for loop_class in side.loop_classes | ||
| if loop_class is not None | ||
| ), |
There was a problem hiding this comment.
When image matching a pair of non-reference datasets, the union radius graph is built from each dataset's learned X_scloop_aligned embedding, but this automatic threshold is derived from loop deaths computed earlier in each dataset's native embedding. MLP/NF mappings are not constrained to preserve distance scale, so the default threshold can be too small to create the required cross-dataset simplices (yielding no matches) or excessively large; calculate the threshold from aligned geometry or require an explicit aligned-space threshold.
Useful? React with 👍 / 👎.
| for ti, tid in enumerate(track_ids): | ||
| for bi, cv in coherence_per_track[tid]: | ||
| presence_matrix[rows_bootstrap[bi], ti] = cv if cv else 0 |
There was a problem hiding this comment.
A loop track can have multiple equivalent candidate matches in the same bootstrap, but each assignment here overwrites the preceding coherence value for that bootstrap. Consequently the Wilcoxon presence result depends on candidate iteration order and ignores all but the last match; group values by (track, bootstrap) and apply an explicit aggregate such as the configured mean or maximum before constructing the matrix.
Useful? React with 👍 / 👎.
| match presence_test_method: | ||
| case "fisher": |
There was a problem hiding this comment.
The previous conditional ended with a ValueError, but this match has no fallback case. A typo or dynamically supplied value in kwargs_loop_test now silently skips the presence test while persistence testing continues, potentially leaving presence_test_result unset or stale and making the pipeline appear successful; retain an explicit default case that rejects unknown methods.
Useful? React with 👍 / 👎.
1 participant
