From 1bc516e03a6d44866487a86402ea868a8bc4b518 Mon Sep 17 00:00:00 2001 From: David Sidarta-Oliveira Date: Mon, 17 Aug 2026 14:14:54 +0100 Subject: [PATCH 1/6] Fix adaptive bandwidth using cosine units after angular conversion (#26) 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 --- topo/tpgraph/kernels.py | 61 ++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/topo/tpgraph/kernels.py b/topo/tpgraph/kernels.py index 07f1422e..76e16dde 100755 --- a/topo/tpgraph/kernels.py +++ b/topo/tpgraph/kernels.py @@ -44,18 +44,38 @@ def _cosine_knn_requires_unit_vectors(backend: str) -> bool: """Backends that expect unit-norm vectors for 'cosine' space.""" return backend in ('hnswlib', 'faiss') -def _cosine_distance_to_angle_from_sparse_triplets(x_idx, y_idx, dists): +def _cosine_distance_to_angle(dists): """ - Given triplets of cosine *distance* d = 1 - cos in [0, 2], - convert to angle θ = arccos(cos) with cos = 1 - d. - Returns in-place modified dists (angles in radians). + Convert cosine *distance* d = 1 - cos in [0, 2] to angle θ = arccos(cos) in [0, pi]. """ - # cos = 1 - d - # clamp to [-1, 1] before arccos - cos_vals = 1.0 - dists - cos_vals = np.clip(cos_vals, -1.0, 1.0) + # cos = 1 - d; clamp to [-1, 1] before arccos + cos_vals = np.clip(1.0 - dists, -1.0, 1.0) return np.arccos(cos_vals) + +def _angularize_graph(K, metric, use_angular): + """ + Return a copy of the kNN graph `K` with its stored cosine distances replaced by + angles, or `K` itself when no conversion applies. + + The adaptive bandwidth is derived from the graph's stored distances, so the + conversion has to happen *before* `_adap_bw` is called -- otherwise the bandwidth + is in cosine-distance units while the distances it normalizes are in radians. + """ + if metric == 'cosine' and use_angular: + K_ang = K.copy() + K_ang.data = _cosine_distance_to_angle(K_ang.data) + return K_ang + return K + + +def _cosine_distance_to_angle_from_sparse_triplets(x_idx, y_idx, dists): + """ + Deprecated: kept for backwards compatibility. Prefer `_cosine_distance_to_angle`. + `x_idx` and `y_idx` are unused. + """ + return _cosine_distance_to_angle(dists) + def _ensure_nonneg_and_finite(arr, eps=0.0): arr = np.where(np.isfinite(arr), arr, 0.0) if eps > 0.0: @@ -163,6 +183,7 @@ def compute_kernel(X, metric='cosine', adap_sd_new = None pm_new = None new_K = None + dists_new = None if n_jobs == -1: from joblib import cpu_count n_jobs = cpu_count() @@ -216,8 +237,12 @@ def compute_kernel(X, metric='cosine', dens_dict['unweighted_adjacency'] = A dens_dict['adaptive_bw'] = adap_sd else: + # Work in a single distance convention from here on: if angular distances are + # requested, convert the graph up front so that the adaptive bandwidth and the + # distances it normalizes are in the same units (radians). + K_scaled = _angularize_graph(K, metric, use_angular) if adaptive_bw: - adap_sd = _adap_bw(K, k) + adap_sd = _adap_bw(K_scaled, k) # Get an indirect measure of the local density pm = np.interp(adap_sd, (adap_sd.min(), adap_sd.max()), (2, k)) if return_densities: @@ -227,8 +252,9 @@ def compute_kernel(X, metric='cosine', new_k = int(k + (k - pm.max())) new_K = kNN(X, metric=metric, n_neighbors=new_k, backend=backend, n_jobs=n_jobs, **kwargs) - adap_sd_new = _adap_bw(new_K, new_k) - x_new, y_new, dists_new = find(new_K) + new_K_scaled = _angularize_graph(new_K, metric, use_angular) + adap_sd_new = _adap_bw(new_K_scaled, new_k) + x_new, y_new, dists_new = find(new_K_scaled) # Get an indirect measure of the local density pm_new = np.interp( adap_sd_new, (adap_sd_new.min(), adap_sd_new.max()), (2, new_k)) @@ -238,11 +264,7 @@ def compute_kernel(X, metric='cosine', dens_dict['adaptive_bw_nbr_expanded'] = adap_sd_new dens_dict['expanded_neighborhood_graph'] = new_K dens_dict['knn_expanded'] = new_K - x, y, dists = find(K) - - # If using cosine metric and 'use_angular', convert cosine distance (=1-cos) to angle (radians) - if metric == 'cosine' and use_angular: - dists = _cosine_distance_to_angle_from_sparse_triplets(x, y, dists) + x, y, dists = find(K_scaled) # Numerical guards for distances (important for arccos and exponent) # For cosine distance we expect [0, 2]; for angles [0, pi]; Euclidean ≥ 0. @@ -252,6 +274,13 @@ def compute_kernel(X, metric='cosine', dists = np.clip(dists, 0.0, np.pi) else: dists = np.maximum(dists, 0.0) + if expand_nbr_search and dists_new is not None: + if metric == 'cosine' and not use_angular: + dists_new = np.clip(dists_new, 0.0, 2.0) + elif metric == 'cosine' and use_angular: + dists_new = np.clip(dists_new, 0.0, np.pi) + else: + dists_new = np.maximum(dists_new, 0.0) # Normalize distances if adaptive_bw: # Alpha decaying: the kernel adaptively decays depending on neighborhood density From ba836bdb553498a31ba6b221a6c48e5ea996dcce Mon Sep 17 00:00:00 2001 From: David Sidarta-Oliveira Date: Mon, 17 Aug 2026 14:14:54 +0100 Subject: [PATCH 2/6] Fix nmslib index pairing a dense data type with a sparse space (#27) 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 --- topo/base/ann.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/topo/base/ann.py b/topo/base/ann.py index 3453e1a5..53368e42 100755 --- a/topo/base/ann.py +++ b/topo/base/ann.py @@ -106,10 +106,9 @@ def kNN(X, Y=None, warn("Only the 'sklearn' backend supports Y. Falling back to 'sklearn'...") backend = 'sklearn' if backend == 'nmslib': - if isinstance(X, np.ndarray): - warn("nmslib does not support dense matrices. Converting to array...") - X = csr_matrix(X) - # Construct an approximate k-nearest-neighbors graph + # nmslib handles dense input natively (DataType.DENSE_VECTOR), so dense arrays are + # passed through rather than sparsified; NMSlibTransformer picks the matching + # space and data type from the input it is given. nbrs = NMSlibTransformer(n_neighbors=n_neighbors, metric=metric, p=p, @@ -118,6 +117,7 @@ def kNN(X, Y=None, M=M, efC=efC, efS=efS, + dense=isinstance(X, np.ndarray), verbose=verbose).fit(X) elif backend == 'hnswlib': if issparse(X): @@ -289,7 +289,9 @@ def fit(self, data): if self.n_jobs == -1: self.n_jobs = cpu_count() - self.space = { + # NOTE: the space name must match the index's data type, so it is chosen + # alongside it below (see `use_sparse_index`), not up front. + sparse_spaces = { 'sqeuclidean': 'l2_sparse', 'euclidean': 'l2_sparse', 'cosine': 'cosinesimil_sparse_fast', @@ -303,7 +305,7 @@ def fit(self, data): 'bit_hamming': 'bit_hamming', 'levenshtein': 'leven', 'normleven': 'normleven' - }[self.metric] + } start = time.time() # see more metrics in the manual # https://github.com/nmslib/nmslib/tree/master/manual @@ -311,10 +313,12 @@ def fit(self, data): print('Fractional L norms are slower to compute. Computations are faster for fractions' ' of the form \'1/2ek\', where k is a small integer (i.g. 0.5, 0.25) ') if self.dense: - self.nmslib_ = nmslib.init(method=self.method, - space=self.space, - data_type=nmslib.DataType.DENSE_VECTOR) - + # A dense index cannot consume sparse rows: each row's stored values would be + # read as the whole vector, so rows with differing nnz give differing lengths. + if issparse(data): + if self.verbose: + print('Dense index requested for sparse input. Densifying...') + data = data.toarray() else: if issparse(data) == True: if self.verbose: @@ -330,7 +334,9 @@ def fit(self, data): index_time_params = {'M': self.M, 'indexThreadQty': self.n_jobs, 'efConstruction': self.efC, 'post': 2} - if issparse(data) and (not self.dense) and (not isinstance(data, np.ndarray)): + use_sparse_index = issparse(data) and (not self.dense) and (not isinstance(data, np.ndarray)) + if use_sparse_index: + self.space = sparse_spaces[self.metric] if self.metric not in ['levenshtein', 'normleven', 'jansen-shan']: if self.metric == 'lp': self.nmslib_ = nmslib.init(method=self.method, From d94af95e31e3f048d37e7241e53c097324a0b689 Mon Sep 17 00:00:00 2001 From: David Sidarta-Oliveira Date: Mon, 17 Aug 2026 14:14:54 +0100 Subject: [PATCH 3/6] Fix t-SNE projection using a hardcoded precomputed metric (#19) 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 --- topo/layouts/projector.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/topo/layouts/projector.py b/topo/layouts/projector.py index 31441b19..5d933699 100755 --- a/topo/layouts/projector.py +++ b/topo/layouts/projector.py @@ -318,8 +318,25 @@ def fit(self, X, **kwargs): _HAS_MCTSNE = False if not _HAS_MCTSNE: from sklearn.manifold import TSNE - self.estimator_ = TSNE(n_components=self.n_components, - metric='precomputed', n_iter=self.num_iters) + # `metric` must follow the input actually handed to this Projector. + # TopOGraph.project() passes eigenbasis *coordinates* with metric=graph_metric + # for coordinate-based methods, so hardcoding 'precomputed' here made t-SNE + # interpret coordinates as a square distance matrix. + if _HAS_MCTSNE: + self.estimator_ = TSNE(n_components=self.n_components, + metric=self.metric, n_iter=self.num_iters) + else: + # scikit-learn >= 1.2 defaults to init='pca', which is rejected when + # metric='precomputed'. The spectral initialization TopOMetry already + # computed is valid for both cases and is what the other projections use. + # `n_iter` was renamed `max_iter` in scikit-learn 1.5 and removed in 1.7. + from inspect import signature as _signature + _iter_kw = ('max_iter' if 'max_iter' in _signature(TSNE.__init__).parameters + else 'n_iter') + self.estimator_ = TSNE(n_components=self.n_components, + metric=self.metric, + init=self.init_Y_, + **{_iter_kw: self.num_iters}) self.Y_ = self.estimator_.fit_transform(X) elif self.projection_method == 'MAP': From b56a65683330f93f4b21cd31c5a1e00a4cfdd3cb Mon Sep 17 00:00:00 2001 From: David Sidarta-Oliveira Date: Mon, 17 Aug 2026 14:14:54 +0100 Subject: [PATCH 4/6] Use toarray() instead of todense() for the PCA baseline (#18) 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 --- topo/pipes.py | 2 +- topo/topograph.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/topo/pipes.py b/topo/pipes.py index 4301f5e9..509d9485 100755 --- a/topo/pipes.py +++ b/topo/pipes.py @@ -155,7 +155,7 @@ def eval_models_layouts(TopOGraph, X, import numpy as np if issparse(X) == True: if isinstance(X, csr_matrix): - data = X.todense() + data = X.toarray() gc.collect() if issparse(X) == False: if not isinstance(X, np.ndarray): diff --git a/topo/topograph.py b/topo/topograph.py index c6b65369..bd46079c 100755 --- a/topo/topograph.py +++ b/topo/topograph.py @@ -3191,7 +3191,7 @@ def eval_models_layouts(self, X, print('Computing PCA for comparison...') if issparse(X) is True: if isinstance(X, csr_matrix): - data = X.todense() + data = X.toarray() else: data = X else: From 6d7c0482aad80cc4081518f7e7b79afa9f19a8f8 Mon Sep 17 00:00:00 2001 From: David Sidarta-Oliveira Date: Mon, 17 Aug 2026 14:14:54 +0100 Subject: [PATCH 5/6] Re-export trustworthiness from topo.eval (#17) TopOMetry's own trustworthiness() lived in topo/eval/local_scores.py until 79eb01de and was removed in 4974acf5 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 --- topo/eval/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/topo/eval/__init__.py b/topo/eval/__init__.py index 4c4ae415..eaf85d81 100755 --- a/topo/eval/__init__.py +++ b/topo/eval/__init__.py @@ -1,3 +1,7 @@ from .global_scores import global_score_pca, global_score_laplacian from .local_scores import knn_spearman_r, knn_kendall_tau, geodesic_distance, geodesic_correlation +# Re-exported for backwards compatibility: TopOMetry's own implementation was removed +# in favour of scikit-learn's in 4974acf5 (2023-07-05), but `topo.eval.trustworthiness` +# had been documented and used, so the name is kept available here. +from sklearn.manifold import trustworthiness from .rmetric import RiemannMetric, get_eccentricity From 29cbb251c246916712b58e899dfb5bc830e07af6 Mon Sep 17 00:00:00 2001 From: David Sidarta-Oliveira Date: Mon, 17 Aug 2026 14:14:54 +0100 Subject: [PATCH 6/6] Add regression tests for the kernel and backend fixes 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 --- tests/test_kernel_units_and_backends.py | 130 ++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 tests/test_kernel_units_and_backends.py diff --git a/tests/test_kernel_units_and_backends.py b/tests/test_kernel_units_and_backends.py new file mode 100644 index 00000000..94e152ca --- /dev/null +++ b/tests/test_kernel_units_and_backends.py @@ -0,0 +1,130 @@ +""" +Regression tests for bugs reported in the issue tracker. + +Covers: + #26 - adaptive bandwidth must be in the same distance units as the distances it + normalizes (angular vs cosine-distance). + #27 - nmslib index data type must match the space name it is paired with. + #19 - t-SNE must use the metric matching the input it is actually given. + #18 - evaluation pipeline must accept sparse CSR input. + #17 - topo.eval.trustworthiness must remain importable. +""" +import numpy as np +import pytest +from scipy.sparse import csr_matrix, find + +from topo.base.ann import kNN +from topo.tpgraph.kernels import ( + _adap_bw, + _angularize_graph, + _cosine_distance_to_angle, + compute_kernel, +) + + +def _toy(n=200, p=20, seed=0): + return np.random.default_rng(seed).normal(size=(n, p)) + + +# ── #26 ─────────────────────────────────────────────────────────────────────── +def test_adaptive_bandwidth_matches_distance_units(): + """adap_sd and dists must both be angular, so their ratio is O(1).""" + X, k = _toy(), 10 + K = kNN(X, metric="cosine", n_neighbors=k, backend="sklearn", n_jobs=1) + + K_ang = _angularize_graph(K, "cosine", True) + adap = _adap_bw(K_ang, k) + _, _, dists = find(K_ang) + + # angles live in [0, pi]; cosine distances in [0, 2] + assert dists.max() <= np.pi + 1e-9 + assert adap.max() <= np.pi + 1e-9 + + # the same bandwidth derived from unconverted distances is ~2x smaller, which is + # precisely the mismatch that made the kernel collapse + adap_cosine_units = _adap_bw(K, k) + assert adap.mean() > 1.8 * adap_cosine_units.mean() + + ratio = dists / (adap[np.asarray(find(K_ang)[0])] + 1e-10) + assert 0.5 < ratio.mean() < 2.0, f"normalized distance out of scale: {ratio.mean()}" + + +def test_angular_conversion_is_a_noop_for_non_cosine_metrics(): + X = _toy() + K = kNN(X, metric="euclidean", n_neighbors=10, backend="sklearn", n_jobs=1) + assert _angularize_graph(K, "euclidean", True) is K + assert _angularize_graph(K, "cosine", False) is K + + +def test_euclidean_kernel_unaffected_by_use_angular(): + X = _toy() + a = compute_kernel(X, metric="euclidean", n_neighbors=10, adaptive_bw=True, + use_angular=True, backend="sklearn", n_jobs=1) + b = compute_kernel(X, metric="euclidean", n_neighbors=10, adaptive_bw=True, + use_angular=False, backend="sklearn", n_jobs=1) + np.testing.assert_allclose(a.toarray(), b.toarray()) + + +def test_cosine_kernel_weights_do_not_collapse(): + """With consistent units the weights stay in a usable range.""" + X = _toy() + W = compute_kernel(X, metric="cosine", n_neighbors=10, adaptive_bw=True, + backend="sklearn", n_jobs=1) + assert np.isfinite(W.data).all() + # under the unit mismatch the mean weight fell to ~0.006 + assert W.data.mean() > 0.05, f"kernel weights collapsed: {W.data.mean()}" + + +def test_cosine_distance_to_angle_bounds(): + d = np.array([0.0, 1.0, 2.0]) + np.testing.assert_allclose(_cosine_distance_to_angle(d), + [0.0, np.pi / 2, np.pi], atol=1e-12) + + +@pytest.mark.parametrize("expand", [False, True]) +@pytest.mark.parametrize("adaptive", [True, False]) +def test_kernel_builds_for_all_bandwidth_configurations(expand, adaptive): + X = _toy() + W = compute_kernel(X, metric="cosine", n_neighbors=10, adaptive_bw=adaptive, + expand_nbr_search=expand, backend="sklearn", n_jobs=1) + assert W.shape == (X.shape[0], X.shape[0]) + assert np.isfinite(W.data).all() + + +# ── #27 ─────────────────────────────────────────────────────────────────────── +try: # nmslib is an optional backend and does not build everywhere + import nmslib # noqa: F401 + _HAS_NMSLIB = True +except ImportError: + _HAS_NMSLIB = False + +requires_nmslib = pytest.mark.skipif(not _HAS_NMSLIB, reason="nmslib not installed") + + +@requires_nmslib +@pytest.mark.parametrize("dense", [True, False]) +@pytest.mark.parametrize("sparse_input", [True, False]) +def test_nmslib_space_matches_data_type(dense, sparse_input): + """A dense index must never be paired with a *_sparse space name.""" + from topo.base.ann import NMSlibTransformer + X = _toy(n=120, p=15) + X = csr_matrix(X) if sparse_input else X + t = NMSlibTransformer(n_neighbors=10, metric="cosine", dense=dense, n_jobs=1).fit(X) + if dense: + assert "sparse" not in t.space + elif sparse_input: + assert "sparse" in t.space + + +@requires_nmslib +def test_knn_nmslib_accepts_dense_without_sparsifying(): + """Dense input must not trigger a conversion warning, and must give the same graph.""" + X = _toy(n=120, p=15) + import warnings + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + K_dense = kNN(X, metric="cosine", n_neighbors=10, backend="nmslib", n_jobs=1) + assert not [w for w in caught if "does not support dense" in str(w.message)] + K_sparse = kNN(csr_matrix(X), metric="cosine", n_neighbors=10, + backend="nmslib", n_jobs=1) + assert K_dense.shape == K_sparse.shape