Fix confirmed bugs from the issue tracker (#17, #18, #19, #26, #27) - #29
Fix confirmed bugs from the issue tracker (#17, #18, #19, #26, #27)#29davisidarta wants to merge 6 commits into
Conversation
The adaptive bandwidth was derived from the kNN graph's stored distances, which for metric='cosine' are cosine distances d = 1 - cos in [0, 2]. The distances it normalizes were then converted to angles in [0, pi] *after* the bandwidth had already been computed, so `dists / adap_sd[x]` divided radians by a cosine-distance quantity. This affected the default configuration: TopOGraph defaults to base_kernel_version='bw_adaptive' and base_metric='cosine', and use_angular is force-enabled for cosine. Measured on 300x20 Gaussian data with k=10, the normalized distance came out 2.16x too large, and because the weights are exp(-d^2) the mean kernel weight collapsed from 0.284 to 0.0064 - a median relative error of ~98%. Neighbour ordering survived (arccos is monotonic in d), which is why the output looked plausible rather than obviously broken. The graph is now converted to angles up front, before the bandwidth is derived, so a single distance convention holds throughout. This also fixes a second defect in the same block: in the expand_nbr_search branch, dists_new received neither the angular conversion nor the [0, pi] clip, so use_angular was silently ignored there. Euclidean kernels are bit-identical to before; cosine kernels change, which is the point - embeddings computed with 1.1.0 on cosine will differ. Reported by @falsetry1514. Co-Authored-By: Claude <noreply@anthropic.com>
Two problems, both in the nmslib backend.
NMSlibTransformer.fit() assigned the *sparse* space table unconditionally before
checking self.dense, so a dense index was initialised with a sparse space name.
That made dense=True raise for every input:
ValueError: The space type cosinesimil_sparse_fast is not compatible with
the type DENSE_VECTOR, only dense vector spaces are allowed!
The dense space table further down was only reachable when dense=False, so the
dense path was effectively dead code. The space name is now chosen alongside the
data type at a single decision point, and a dense index densifies sparse input
rather than handing nmslib rows of differing length.
kNN() also force-converted dense arrays to CSR with a warning claiming "nmslib
does not support dense matrices" - which is false, nmslib has
DataType.DENSE_VECTOR - and which said "Converting to array" while converting to
a sparse matrix. Dense input is now passed through untouched.
Reported by @falsetry1514.
Co-Authored-By: Claude <noreply@anthropic.com>
The t-SNE branch hardcoded metric='precomputed', but TopOGraph.project() hands coordinate-based methods the *eigenbasis coordinates* together with metric=graph_metric. t-SNE was therefore told to read a coordinate matrix as a square distance matrix. It now honours self.metric, like the other projections. That hardcoded metric is also what surfaced the originally reported error: with metric='precomputed', scikit-learn >= 1.2 rejects its own new default init='pca'. The reported ValueError is fixed, but init='random' would only have masked the real problem, so the spectral initialisation TopOMetry already computes is passed explicitly instead - a better starting point and valid for both metric settings. Separately, `n_iter` was renamed `max_iter` in scikit-learn 1.5 and removed in 1.7, so on current scikit-learn the call failed with a TypeError before it ever reached the init check. The keyword is now selected by introspection. The MulticoreTSNE path is updated for consistency but is untested here, as that package does not build on Python 3.12. Reported by @droully. Co-Authored-By: Claude <noreply@anthropic.com>
csr_matrix.todense() returns np.matrix, which current scikit-learn rejects
outright:
TypeError: np.matrix is not supported. Please convert to a numpy array
with np.asarray.
so eval_models_layouts() failed for any sparse input. Fixed at both sites, as
reported. topo/utils/_utils.py already wraps its todense() in np.asarray, so it
was unaffected.
Reported by @sciwithrach, who also supplied the fix.
Co-Authored-By: Claude <noreply@anthropic.com>
TopOMetry's own trustworthiness() lived in topo/eval/local_scores.py until 79eb01d and was removed in 4974acf in favour of scikit-learn's, which topo/pipes.py has used since. That was a reasonable call, but the name had been documented and used, and nothing was re-exported - so from the outside it just looked missing. Reported by @daniel-spies. Co-Authored-By: Claude <noreply@anthropic.com>
Covers the unit consistency of the adaptive bandwidth (#26) and the nmslib space/data-type pairing (#27). Two of these fail on master for behavioural reasons rather than by construction: the cosine weight-collapse assertion sees a mean weight of 0.0064 against a threshold of 0.05, and the dense nmslib index raises ValueError. The nmslib cases are skipped individually when the optional backend is absent, so the kernel tests still run in an environment without it. Co-Authored-By: Claude <noreply@anthropic.com>
4002338 to
29cbb25
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4002338437
ℹ️ 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 metric == 'cosine' and use_angular: | ||
| K_ang = K.copy() | ||
| K_ang.data = _cosine_distance_to_angle(K_ang.data) |
There was a problem hiding this comment.
Use the actual cosine similarity when angularizing
For the default compute_kernel(..., metric='cosine') path, K comes from kNN, which has already inverted sklearn/nmslib cosine distances with knn.data = 1 - knn.data, so K.data is cosine similarity rather than a cosine distance. Passing those values to _cosine_distance_to_angle computes arccos(1 - similarity): duplicate/self edges with similarity 1 become π/2 and opposite edges become 0, so the newly converted adaptive bandwidths and weights are still in the wrong units for cosine kernels. Convert from the stored similarity (arccos(K.data)) or stop inverting before this helper.
Useful? React with 👍 / 👎.
| if issparse(data): | ||
| if self.verbose: | ||
| print('Dense index requested for sparse input. Densifying...') | ||
| data = data.toarray() |
There was a problem hiding this comment.
Densify sparse queries for dense nmslib indexes
When NMSlibTransformer(dense=True) is fit on a CSR matrix, this densifies only the local data used to build a DENSE_VECTOR index. The normal transformer flow (fit_transform(csr) or fit(csr).transform(csr)) still passes the original sparse matrix into transform, even though the nearby comment notes dense indexes cannot consume sparse rows, so the newly supported dense+sparse combination fails at query time. Record that the fit data was densified and apply the same conversion in transform and ind_dist_grad.
Useful? React with 👍 / 👎.
Fixes five bugs reported in the tracker, each verified reproduced before and fixed after.
One commit per issue.
computed with 1.1.0 using the default
base_metric='cosine'will differ after thismerge. That is the fix working, not a regression, but it warrants a version bump and a
changelog note.
#26 — adaptive bandwidth in the wrong distance units (highest impact)
adap_sdwas derived from the kNN graph's stored cosine distances (d = 1 - cos,range
[0, 2]), while the distances it normalises were converted to angles(
[0, π]) only afterwards.dists / adap_sd[x]therefore divided radians by acosine-distance quantity.
This hit the default configuration —
base_kernel_version='bw_adaptive',base_metric='cosine', anduse_angularforce-enabled for cosine. Measured on 300×20Gaussian data, k=10:
A 2.16× inflation in the exponent of
exp(-d²)collapses the weights. Neighbourordering survived because arccos is monotonic in
d, which is why results lookedplausible instead of obviously wrong — and why this went unnoticed since 0168865
(2025-09-09), i.e. for all of 1.1.0.
The graph is now converted to angles before the bandwidth is derived, so one distance
convention holds throughout. This also fixes a second defect in the same block: in the
expand_nbr_searchbranch,dists_newgot neither the conversion nor the[0, π]clip,so
use_angularwas silently ignored there.Verified: euclidean kernels are bit-identical to master; cosine kernels change as above.
#27 — nmslib dense index paired with a sparse space
NMSlibTransformer.fit()assigned the sparse space table unconditionally before checkingself.dense, sodense=Trueraised for every input, dense or sparse:The dense space table was only reachable when
dense=False, making the dense path deadcode. Space name and data type are now chosen together at one decision point.
kNN()also force-converted dense arrays to CSR under a warning claiming "nmslib does notsupport dense matrices" — false, nmslib has
DataType.DENSE_VECTOR— which additionallysaid "Converting to array" while converting to sparse. Dense input now passes through.
Verified all four combinations of
dense× sparse/dense input build with a correctlypaired space (
cosinesimilvscosinesimil_sparse_fast).#19 — t-SNE hardcoded
metric='precomputed'The root cause was deeper than reported.
TopOGraph.project()hands coordinate-basedmethods the eigenbasis coordinates with
metric=graph_metric, but the t-SNE branchhardcoded
metric='precomputed'— so t-SNE read coordinates as a square distance matrix.It now honours
self.metric.That hardcoded metric is what surfaced the reported error: with
precomputed, sklearn≥ 1.2 rejects its own new default
init='pca'.init='random'would have masked the realbug, so the spectral initialisation TopOMetry already computes is passed instead.
Also:
n_iter→max_iter(renamed in sklearn 1.5, removed in 1.7), selected byintrospection. On sklearn 1.9 the old call failed with
TypeErrorbefore even reaching theinit check.
The MulticoreTSNE path is updated for consistency but untested — it does not build on
Python 3.12.
#18 —
todense()returnsnp.matrixeval_models_layouts()failed for sparse input because current sklearn rejectsnp.matrixoutright. Fixed at both sites (pipes.py,topograph.py) exactly as thereporter suggested.
utils/_utils.pyalready wraps itstodense()innp.asarray.#17 —
trustworthinessnot exportedRemoved from
local_scores.pyin 4974acf in favour of sklearn's, but never re-exporteddespite having been documented. Now available from
topo.evalagain.Tests
New
tests/test_kernel_units_and_backends.py(14 tests). Two fail on master forbehavioural rather than structural reasons — the weight-collapse assertion sees 0.0064
against a 0.05 threshold, and the dense nmslib index raises. nmslib cases skip
individually when the backend is absent, so kernel coverage survives without it.
Full suite: 39 passed (25 existing + 14 new), no regressions. Existing tests need
scikit-miscandhnswlibinstalled.Not fixed here — reported separately
expand_nbr_searchis a no-op.new_k = int(k + (k - pm.max())), andpm.max()is always exactly
kby construction of thenp.interprange, sonew_k == kalways.The feature recomputes the same graph and expands nothing. Choosing what
new_kshould be is a design decision, so it is left alone.
use_angular=Falseis silently ignored. The override atkernels.py:173tests'use_angular' not in kwargs, butuse_angularis a named parameter and so neverappears in
kwargs— passing it explicitly has no effect for cosine. Harmless giventhe corrected units are now the right default, but the flag does not work.
scikit-miscandhnswlib;scikit-miscis required by scanpy'sseurat_v3HVG flavor, which is TopOMetry'sdefault.