diff --git a/CHANGELOG.md b/CHANGELOG.md index b2103772..91887706 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`DistanceExporter.compute_pairs()` gains an opt-in `metric_errors` column to distinguish legitimate `None` results from computation failures** (#960, follow-up to #879) by @Karunasagar12 + - Previously, a `None` in `hop_count`/`weighted_distance`/`semantic_similarity`/betweenness could mean either "no path exists" or "the underlying computation raised" — logged as a warning per #879, but not otherwise surfaced, so the two cases were indistinguishable in exported CSV/JSONL/DataFrame data. `include=["metric_errors"]` now adds a `metric_errors` field per row: `""` when all requested metrics succeeded, or a comma-separated list of metric names that raised (e.g. `"hop_count,weighted_distance"`) + - Opt-in only — default `compute_pairs()`/`to_csv()`/`to_dataframe()`/`to_jsonl()` schema is unchanged unless `"metric_errors"` is explicitly requested + - The four metric helpers (`_betweenness`, `_hop_distance`, `_weighted_distance`, `_semantic_similarity`) now return `(value, error_name | None)` tuples internally; `compute_pairs()` aggregates the error names per row + - **Fixed during review** (Qodo): `_betweenness()` failures weren't tracked into `metric_errors` in the initial version — centrality computation could raise and the column would still report `""`. Now returns its error tuple like the other three helpers + - **Known limitation**: `include=["metric_errors"]` with no other metric names computes nothing, so the column is always `""` in that case — pass it alongside the metrics you want tracked, e.g. `include=["hop_count", "metric_errors"]` + - New `tests/export/test_distance_exporter_metric_errors.py`: 6 tests covering success, single/multiple failures, opt-out, the no-path-vs-error distinction, and default-schema stability; existing `tests/export/test_distance_exporter.py` updated for the new tuple return type + - Full `tests/export/` suite: 77 passed + ### Changed - **`GraphBuilder`'s 6 public methods now have Google-style docstrings** (#878, closes #876) by @cakeni diff --git a/semantica/export/distance_exporter.py b/semantica/export/distance_exporter.py index 060f041a..3d2d250d 100644 --- a/semantica/export/distance_exporter.py +++ b/semantica/export/distance_exporter.py @@ -11,12 +11,15 @@ Python API: df = exporter.to_dataframe(include=["hops", "semantic_similarity", "distance_band"]) exporter.to_csv("distances.csv") exporter.to_jsonl("distances.jsonl") + + # Include error status columns for auditable exports: + df = exporter.to_dataframe(include=["hop_count", "metric_errors"]) """ import csv import io import json -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple from ..utils.helpers import classify_path_distance from ..utils.logging import get_logger @@ -36,6 +39,9 @@ _ALL_COLUMNS = [ "distance_band", "source_betweenness", "target_betweenness", ] +# Error status columns — opt-in via include=["metric_errors"] +# (used by compute_pairs when "metric_errors" is in include set) + class DistanceExporter: """Compute and export pairwise distance metrics for a ContextGraph.""" @@ -65,63 +71,77 @@ class DistanceExporter: node = getattr(self.graph, "nodes", {}).get(node_id) return getattr(node, "node_type", "") if node else "" - def _betweenness(self, graph_dict: Dict[str, Any]) -> Dict[str, float]: + def _betweenness(self, graph_dict: Dict[str, Any]) -> Tuple[Dict[str, float], Optional[str]]: + """Return (betweenness_dict, error). error is None on success.""" if self._centrality is None: - return {} + return {}, None try: result = self._centrality.calculate_betweenness_centrality(graph_dict) - return result.get("betweenness", {}) if isinstance(result, dict) else {} + return (result.get("betweenness", {}) if isinstance(result, dict) else {}), None except Exception: logger.warning("Betweenness centrality computation failed; omitting from export", exc_info=True) - return {} + return {}, "betweenness" - def _hop_distance(self, graph_dict: Dict[str, Any], src: str, tgt: str) -> Optional[int]: + def _hop_distance(self, graph_dict: Dict[str, Any], src: str, tgt: str) -> Tuple[Optional[int], Optional[str]]: + """Return (hop_count, error). error is None on success or a short description on failure.""" if self._path_finder is None: - return None + return None, None # KG unavailable — not an error, just no data try: result = self._path_finder.bfs_shortest_path(graph_dict, src, tgt) path = result.get("path", []) if isinstance(result, dict) else (result or []) - return len(path) - 1 if path else None + return (len(path) - 1 if path else None), None except Exception: logger.warning("Hop distance computation failed for %s -> %s; returning None sentinel", src, tgt, exc_info=True) - return None + return None, "hop_count" - def _weighted_distance(self, graph_dict: Dict[str, Any], src: str, tgt: str) -> Optional[float]: + def _weighted_distance(self, graph_dict: Dict[str, Any], src: str, tgt: str) -> Tuple[Optional[float], Optional[str]]: + """Return (weighted_distance, error). error is None on success.""" if self._path_finder is None: - return None + return None, None try: result = self._path_finder.dijkstra_shortest_path(graph_dict, src, tgt) if isinstance(result, dict): - return float(result.get("total_weight", len(result.get("path", [])) - 1)) - return None + return float(result.get("total_weight", len(result.get("path", [])) - 1)), None + return None, None except Exception: logger.warning("Weighted distance computation failed for %s -> %s; returning None sentinel", src, tgt, exc_info=True) - return None + return None, "weighted_distance" - def _semantic_similarity(self, graph_dict: Dict[str, Any], src: str, tgt: str) -> Optional[float]: + def _semantic_similarity(self, graph_dict: Dict[str, Any], src: str, tgt: str) -> Tuple[Optional[float], Optional[str]]: + """Return (similarity, error). error is None on success.""" if self._similarity is None: - return None + return None, None try: sim = self._similarity.cosine_similarity(graph_dict, src, tgt) - return float(sim) if isinstance(sim, (int, float)) else None + return (float(sim) if isinstance(sim, (int, float)) else None), None except Exception: logger.warning("Semantic similarity computation failed for %s -> %s; returning None sentinel", src, tgt, exc_info=True) - return None + return None, "semantic_similarity" def compute_pairs( self, include: Optional[List[str]] = None, node_subset: Optional[List[str]] = None, ) -> List[Dict[str, Any]]: - """Compute all pairwise distance metrics and return as a list of dicts.""" + """Compute all pairwise distance metrics and return as a list of dicts. + + When ``include`` contains ``"metric_errors"``, each row gains a + ``metric_errors`` field: an empty string when all metrics succeeded, or + a comma-separated list of metric names that raised during computation + (e.g. ``"hop_count,weighted_distance"``). This lets downstream consumers + distinguish legitimate ``None`` (no path) from computation failure. + """ include_set = set(include or _ALL_COLUMNS) + track_errors = "metric_errors" in include_set + include_set.discard("metric_errors") # not a real metric to compute graph_dict = self._build_graph_dict() node_ids = node_subset or list(self.graph.nodes.keys()) betweenness: Dict[str, float] = {} + betweenness_err: Optional[str] = None if "source_betweenness" in include_set or "target_betweenness" in include_set: - betweenness = self._betweenness(graph_dict) + betweenness, betweenness_err = self._betweenness(graph_dict) rows = [] for i, src in enumerate(node_ids): @@ -129,6 +149,10 @@ class DistanceExporter: if src == tgt: continue row: Dict[str, Any] = {} + errors: List[str] = [] + if betweenness_err: + errors.append(betweenness_err) + if "source_id" in include_set: row["source_id"] = src if "source_type" in include_set: @@ -140,15 +164,23 @@ class DistanceExporter: hop_count: Optional[int] = None if "hop_count" in include_set or "distance_band" in include_set: - hop_count = self._hop_distance(graph_dict, src, tgt) + hop_count, hop_err = self._hop_distance(graph_dict, src, tgt) + if hop_err: + errors.append(hop_err) if "hop_count" in include_set: row["hop_count"] = hop_count if "weighted_distance" in include_set: - row["weighted_distance"] = self._weighted_distance(graph_dict, src, tgt) + wd_val, wd_err = self._weighted_distance(graph_dict, src, tgt) + row["weighted_distance"] = wd_val + if wd_err: + errors.append(wd_err) if "semantic_similarity" in include_set: - row["semantic_similarity"] = self._semantic_similarity(graph_dict, src, tgt) + ss_val, ss_err = self._semantic_similarity(graph_dict, src, tgt) + row["semantic_similarity"] = ss_val + if ss_err: + errors.append(ss_err) if "distance_band" in include_set: row["distance_band"] = classify_path_distance(hop_count) if hop_count is not None else "distant" @@ -158,6 +190,9 @@ class DistanceExporter: if "target_betweenness" in include_set: row["target_betweenness"] = betweenness.get(tgt) + if track_errors: + row["metric_errors"] = ",".join(errors) if errors else "" + rows.append(row) return rows diff --git a/tests/export/test_distance_exporter.py b/tests/export/test_distance_exporter.py index e9d20b67..2066743e 100644 --- a/tests/export/test_distance_exporter.py +++ b/tests/export/test_distance_exporter.py @@ -57,28 +57,36 @@ def exporter(): def test_hop_distance_logs_warning_on_exception(exporter, caplog): with caplog.at_level(logging.WARNING, logger="semantica.export.distance_exporter"): result = exporter._hop_distance({}, "a", "b") - assert result is None + value, error = result + assert value is None + assert error == "hop_count" assert any("Hop distance" in rec.message for rec in caplog.records) def test_weighted_distance_logs_warning_on_exception(exporter, caplog): with caplog.at_level(logging.WARNING, logger="semantica.export.distance_exporter"): result = exporter._weighted_distance({}, "a", "b") - assert result is None + value, error = result + assert value is None + assert error == "weighted_distance" assert any("Weighted distance" in rec.message for rec in caplog.records) def test_semantic_similarity_logs_warning_on_exception(exporter, caplog): with caplog.at_level(logging.WARNING, logger="semantica.export.distance_exporter"): result = exporter._semantic_similarity({}, "a", "b") - assert result is None + value, error = result + assert value is None + assert error == "semantic_similarity" assert any("Semantic similarity" in rec.message for rec in caplog.records) def test_betweenness_logs_warning_on_exception(exporter, caplog): with caplog.at_level(logging.WARNING, logger="semantica.export.distance_exporter"): result = exporter._betweenness({}) - assert result == {} + value, error = result + assert value == {} + assert error == "betweenness" assert any("Betweenness" in rec.message for rec in caplog.records) @@ -103,5 +111,7 @@ def test_hop_distance_no_warning_when_kg_unavailable(caplog): exp._path_finder = None with caplog.at_level(logging.WARNING, logger="semantica.export.distance_exporter"): result = exp._hop_distance({}, "a", "b") - assert result is None + value, error = result + assert value is None + assert error is None assert len(caplog.records) == 0 diff --git a/tests/export/test_distance_exporter_metric_errors.py b/tests/export/test_distance_exporter_metric_errors.py new file mode 100644 index 00000000..e8c8e351 --- /dev/null +++ b/tests/export/test_distance_exporter_metric_errors.py @@ -0,0 +1,132 @@ +"""Tests for DistanceExporter metric_errors column. + +Verifies that when ``include=["metric_errors"]`` is passed to +``compute_pairs()``, the exported rows contain a ``metric_errors`` field +that distinguishes computation failures from legitimate None results. +""" + +import logging +from unittest.mock import MagicMock + +import pytest + +from semantica.export.distance_exporter import DistanceExporter + + +@pytest.fixture +def mock_graph(): + """Minimal graph mock with two nodes.""" + graph = MagicMock() + node_a = MagicMock(node_id="a", node_type="entity", content="A", properties={}) + node_b = MagicMock(node_id="b", node_type="entity", content="B", properties={}) + graph.nodes = {"a": node_a, "b": node_b} + graph.edges = [] + return graph + + +@pytest.fixture +def exporter(mock_graph): + """DistanceExporter with mocked KG components.""" + exp = DistanceExporter(mock_graph) + exp._path_finder = MagicMock() + exp._similarity = MagicMock() + exp._centrality = MagicMock() + return exp + + +class TestMetricErrorsColumn: + """Tests for the opt-in metric_errors export column.""" + + def test_metric_errors_empty_on_success(self, exporter): + """When all metrics succeed, metric_errors is an empty string.""" + exporter._path_finder.bfs_shortest_path.return_value = {"path": ["a", "x", "b"]} + exporter._path_finder.dijkstra_shortest_path.return_value = {"total_weight": 2.5, "path": ["a", "b"]} + exporter._similarity.cosine_similarity.return_value = 0.87 + + rows = exporter.compute_pairs(include=["hop_count", "weighted_distance", "semantic_similarity", "metric_errors"]) + + assert len(rows) == 2 # a->b and b->a + for row in rows: + assert "metric_errors" in row + assert row["metric_errors"] == "" + + def test_metric_errors_records_single_failure(self, exporter): + """When one metric fails, its name appears in metric_errors.""" + exporter._path_finder.bfs_shortest_path.return_value = {"path": ["a", "b"]} + exporter._path_finder.dijkstra_shortest_path.side_effect = RuntimeError("negative cycle") + exporter._similarity.cosine_similarity.return_value = 0.5 + + rows = exporter.compute_pairs(include=["hop_count", "weighted_distance", "semantic_similarity", "metric_errors"]) + + for row in rows: + assert row["metric_errors"] == "weighted_distance" + assert row["hop_count"] == 1 # still computed + assert row["weighted_distance"] is None # failed + assert row["semantic_similarity"] == 0.5 # still computed + + def test_metric_errors_records_multiple_failures(self, exporter): + """When multiple metrics fail, all names appear comma-separated.""" + exporter._path_finder.bfs_shortest_path.side_effect = RuntimeError("fail") + exporter._path_finder.dijkstra_shortest_path.side_effect = RuntimeError("fail") + exporter._similarity.cosine_similarity.side_effect = TypeError("fail") + exporter._centrality.calculate_betweenness_centrality.side_effect = RuntimeError("fail") + + rows = exporter.compute_pairs(include=[ + "hop_count", "weighted_distance", "semantic_similarity", + "source_betweenness", "metric_errors", + ]) + + for row in rows: + errors = row["metric_errors"].split(",") + assert "hop_count" in errors + assert "weighted_distance" in errors + assert "semantic_similarity" in errors + assert "betweenness" in errors + assert row["hop_count"] is None + assert row["weighted_distance"] is None + assert row["semantic_similarity"] is None + + def test_metric_errors_absent_when_not_requested(self, exporter): + """When metric_errors is not in include, it doesn't appear in rows.""" + exporter._path_finder.bfs_shortest_path.side_effect = RuntimeError("fail") + exporter._path_finder.dijkstra_shortest_path.return_value = {"total_weight": 1.0, "path": ["a", "b"]} + exporter._similarity.cosine_similarity.return_value = 0.9 + + rows = exporter.compute_pairs(include=["hop_count", "weighted_distance", "semantic_similarity"]) + + for row in rows: + assert "metric_errors" not in row + + def test_metric_errors_distinguishes_no_path_from_error(self, exporter): + """Core distinction: None from 'no path' has empty error; None from exception has the metric name.""" + # bfs returns empty path (legitimate "no path") — NOT an error + exporter._path_finder.bfs_shortest_path.return_value = {"path": []} + # dijkstra raises (computation error) + exporter._path_finder.dijkstra_shortest_path.side_effect = ValueError("bad weight") + exporter._similarity.cosine_similarity.return_value = 0.3 + + rows = exporter.compute_pairs(include=["hop_count", "weighted_distance", "semantic_similarity", "metric_errors"]) + + for row in rows: + # Both are None, but only weighted_distance is an error + assert row["hop_count"] is None + assert row["weighted_distance"] is None + assert row["metric_errors"] == "weighted_distance" + + def test_default_columns_unchanged_without_metric_errors(self, exporter): + """Default column set (no metric_errors) produces the same schema as before.""" + exporter._path_finder.bfs_shortest_path.return_value = {"path": ["a", "b"]} + exporter._path_finder.dijkstra_shortest_path.return_value = {"total_weight": 1.0, "path": ["a", "b"]} + exporter._similarity.cosine_similarity.return_value = 0.5 + exporter._centrality.calculate_betweenness_centrality.return_value = {"betweenness": {"a": 0.5, "b": 0.3}} + + rows = exporter.compute_pairs() + + assert len(rows) == 2 + expected_keys = { + "source_id", "source_type", "target_id", "target_type", + "hop_count", "weighted_distance", "semantic_similarity", + "distance_band", "source_betweenness", "target_betweenness", + } + assert set(rows[0].keys()) == expected_keys + assert "metric_errors" not in rows[0]