loop densification and coherence - #110
Conversation
![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: 34c1ec2006
ℹ️ 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".
| result.homotopy_coherence_matched_relax = [ | ||
| compute_coherence( | ||
| filling=source_fillings[source_index], | ||
| deformation=deformation["triangle_ids"], | ||
| triangle_areas=triangle_areas, | ||
| ) |
There was a problem hiding this comment.
When a match is produced by the relaxed solver, its triangle chain has boundary equal to the source/target difference only after including deformation["relaxation_edge_ids"]. Computing coherence from triangle_ids alone therefore treats a chain that does not connect the two loops as a valid homotopy, producing incorrect values for relaxed matches and consequently incorrect Wilcoxon presence-test inputs when that method is selected. Incorporate the relaxation edges into the coherence calculation or exclude relaxed matches from this metric.
Useful? React with 👍 / 👎.
| if local_scale is None: | ||
| nn = NearestNeighbors(n_neighbors=k_local_scale + 1).fit(embedding) | ||
| knn_distances, _ = nn.kneighbors(embedding) | ||
| local_scale = np.asarray(knn_distances[:, 1:].mean(axis=1), dtype=np.float64) |
There was a problem hiding this comment.
With the default k_local_scale=5, any embedding containing fewer than six observations causes NearestNeighbors.kneighbors to raise because n_neighbors exceeds n_samples. find_loops() invokes this refinement unconditionally, even when no loop representatives were found, so otherwise valid small datasets fail at the end of the pipeline. Clamp the requested neighbor count to the available observations, and handle the single-observation case.
Useful? React with 👍 / 👎.
| representatives=representatives, | ||
| representatives_refined=representatives_refined, | ||
| representatives_valid=super_obj.representatives_valid, |
There was a problem hiding this comment.
When representatives_valid contains false entries, from_super() still copies every representative and its derived coordinate, gradient, and edge arrays, while only attaching the validity mask afterward. The Hodge pipeline consequently builds masks for these invalid cycles and includes their edges in smoothing, divergence, trajectory identification, and gene-trend computations; filtering them only while plotting cannot prevent them from changing the analyzed results. Filter all per-representative inputs consistently before constructing LoopClassAnalysis.
Useful? React with 👍 / 👎.
| ) -> list[tuple[Index_t, ...] | None]: | ||
| if self._cached_fillings is not None: | ||
| return self._cached_fillings |
There was a problem hiding this comment.
After fillings() is called once, every later call returns the same cached chain even if it supplies a different boundary matrix, column_trim_method, or column_scores. Reusing a LoopClass for another bootstrap/equivalence analysis with different trimming settings therefore computes coherence from a filling selected under the previous settings. Either key the cache by all inputs that affect the solve or invalidate/recompute it when those inputs change.
Useful? React with 👍 / 👎.
| for u, v in zip(vertices[:-1], vertices[1:]): | ||
| poly = [u, v] | ||
| lengths = [float(np.linalg.norm(embedding[u] - embedding[v]))] |
There was a problem hiding this comment.
Reconstructed representatives normally omit a repeated first vertex and are closed implicitly by loops_to_coords() and loops_to_edge_mask(), but this iteration processes only adjacent entries and never visits the edge from vertices[-1] back to vertices[0]. Consequently use_refined=True can still render one arbitrarily long unsplit segment—the cocycle edge that closes the loop—while every other segment is densified. Include the wraparound pair in refinement while preserving the representation's expected closure format.
Useful? React with 👍 / 👎.
1 participant
Closes #96