From 50c0938508023d431395afda5e742a110097359a Mon Sep 17 00:00:00 2001 From: MaartenGr Date: Sat, 22 Aug 2026 08:25:39 +0200 Subject: [PATCH] Fix isolated bugs in the topic data layer --- bertopic/_bertopic.py | 31 +++++++++++++--------------- bertopic/_topics.py | 43 +++++++++++++++++++++++++-------------- tests/test_topics.py | 47 +++++++++++++++++++------------------------ 3 files changed, 63 insertions(+), 58 deletions(-) diff --git a/bertopic/_bertopic.py b/bertopic/_bertopic.py index 25439b8b..80c56761 100644 --- a/bertopic/_bertopic.py +++ b/bertopic/_bertopic.py @@ -334,7 +334,8 @@ def topic_representations_(self) -> dict[int, list[tuple[str, float]]]: @property def topic_aspects_(self) -> dict[int, dict[str, list[tuple[str, float]]]]: """For backwards compatibility.""" - aspect_names = list(self._topics[0].representations.keys()) + first_topic = next(iter(self._topics), None) + aspect_names = list(first_topic.representations) if first_topic else [] aspects = { aspect_name: {topic.id: topic.representations[aspect_name].data for topic in self._topics} for aspect_name in aspect_names @@ -372,7 +373,7 @@ def representative_docs_(self) -> dict[int, str]: return { topic.id: topic.representative_documents for topic in self._topics - if topic.representative_documents is not None + if topic.representative_documents } @property @@ -381,7 +382,7 @@ def representative_images_(self) -> dict[int, str]: return { topic.id: topic.representative_images for topic in self._topics - if topic.representative_images is not None + if topic.representative_images.size > 0 } @property @@ -2379,14 +2380,6 @@ def _images_to_text(self, corpus: Corpus) -> pd.DataFrame: logger.info("Images - Completed \u2713") return corpus - def _map_predictions(self, predictions: List[int]) -> List[int]: - """Map predictions to the correct topics if topics were reduced.""" - mappings = self.topic_mapper_.get_mappings(original_topics=True) - mapped_predictions = [ - mappings[prediction] if prediction in mappings else -1 for prediction in predictions - ] - return mapped_predictions - def _reduce_dimensionality( self, corpus: Corpus, @@ -2964,10 +2957,12 @@ def _reduce_to_n_topics(self, corpus: Corpus, use_ctfidf: bool = False) -> Corpu else: cluster = AgglomerativeClustering(self.nr_topics, affinity="precomputed", linkage="average") cluster.fit(distance_matrix) - new_topics = [cluster.labels_[topic] if topic != -1 else -1 for topic in corpus.topics] - # Track mappings and group them - mappings = {from_topic: to_topic for from_topic, to_topic in zip(corpus.topics, new_topics)} + # Map every topic, not just those holding documents, so no topic is left behind + mappings = { + topic_id: -1 if topic_id == -1 else int(cluster.labels_[topic_id]) + for topic_id in self._topics.topic_ids() + } self._topics.merge(mappings) corpus.map_topics_and_probabilities(self._topics, from_original=False) @@ -3028,9 +3023,11 @@ def _auto_reduce_topics(self, corpus: Corpus, use_ctfidf: bool = False) -> Corpu else: mapped_topics[topic_id] = cluster_to_lowest[cluster] - # Track mappings and group them - new_topics = [mapped_topics[topic] if topic != -1 else -1 for topic in corpus.topics] - mappings = {from_topic: to_topic for from_topic, to_topic in zip(corpus.topics, new_topics)} + # Map every topic, not just those holding documents, so no topic is left behind + mappings = { + topic_id: -1 if topic_id == -1 else mapped_topics[topic_id] + for topic_id in self._topics.topic_ids() + } self._topics.merge(mappings) corpus.map_topics_and_probabilities(self._topics, from_original=False) diff --git a/bertopic/_topics.py b/bertopic/_topics.py index 0ac09dc1..6ca20464 100644 --- a/bertopic/_topics.py +++ b/bertopic/_topics.py @@ -1,3 +1,4 @@ +from copy import deepcopy from dataclasses import dataclass, field from enum import Enum from typing import Any @@ -184,6 +185,12 @@ def apply(self, new_mapping: dict[int, int]) -> None: self._mapping = new_mapping.copy() self._recent_mapping = new_mapping.copy() else: + missing = sorted(set(self._mapping.values()) - set(new_mapping)) + if missing: + raise ValueError( + f"The mapping is missing topics {missing}. Every current topic must appear " + "in a new mapping, including topics that hold no documents." + ) self._mapping = {original: new_mapping[current] for original, current in self._mapping.items()} self._recent_mapping = new_mapping.copy() @@ -376,13 +383,12 @@ def to_dict(self, full: bool = False) -> dict: if full: data["embedding"] = self.embedding.tolist() if self.embedding.size else [] - if self.c_tf_idf.nnz > 0: - data["c_tf_idf"] = { - "data": self.c_tf_idf.data.tolist(), - "indices": self.c_tf_idf.indices.tolist(), - "indptr": self.c_tf_idf.indptr.tolist(), - "shape": list(self.c_tf_idf.shape), - } + data["c_tf_idf"] = { + "data": self.c_tf_idf.data.tolist(), + "indices": self.c_tf_idf.indices.tolist(), + "indptr": self.c_tf_idf.indptr.tolist(), + "shape": list(self.c_tf_idf.shape), + } if self.representative_images.size: data["representative_images"] = self.representative_images.tolist() @@ -438,7 +444,7 @@ def from_dict(cls, data: dict) -> "Topic": def copy(self, new_id: int | None = None) -> "Topic": """Create a copy of this topic, optionally with a new ID.""" - copied = Topic.from_dict(self.to_dict(full=True)) + copied = deepcopy(self) if new_id is not None: copied.id = new_id return copied @@ -706,12 +712,19 @@ def merge(self, old_to_new: dict[int, int]) -> None: old_topics = [self.topics[old_id] for old_id in old_ids] total_docs = sum(t.nr_documents for t in old_topics) - # Calculate weights (handle zero documents edge case) - weights = np.array([t.nr_documents / total_docs for t in old_topics]) + # Weight by document count, falling back to equal weights when a group holds + # no documents at all, which happens transiently during `partial_fit` + if total_docs: + weights = np.array([t.nr_documents / total_docs for t in old_topics]) + else: + weights = np.full(len(old_topics), 1 / len(old_topics)) - # Weighted average of embeddings - if old_topics[0].embedding.size > 0: - embedding = np.average([t.embedding for t in old_topics], weights=weights, axis=0) + # Weighted average of embeddings, which are optional and may never have been computed + embeddings = [t.embedding for t in old_topics if t.embedding.size > 0] + if len(embeddings) == len(old_topics): + embedding = np.average(embeddings, weights=weights, axis=0) + else: + embedding = np.array([]) # Weighted average of c-TF-IDF vectors (sparse) c_tf_idf = sum(t.c_tf_idf * w for t, w in zip(old_topics, weights)) @@ -1160,10 +1173,10 @@ def to_dict(self) -> dict: """Serialize hierarchy for storage.""" return { "bertopic_version": BERTOPIC_VERSION, - "nodes": {str(nid): node.to_dict() for nid, node in self.nodes.items()}, + "nodes": {str(nid): node.to_dict(full=True) for nid, node in self.nodes.items()}, "linkage_matrix": self.linkage_matrix.tolist() if self.linkage_matrix.size > 0 else [], "n_leaves": self.n_leaves, - "outlier_topic": self.outlier_topic.to_dict() if self.outlier_topic else None, + "outlier_topic": self.outlier_topic.to_dict(full=True) if self.outlier_topic else None, "predictions": self._original_predictions.tolist() if self._original_predictions.size > 0 else [], } diff --git a/tests/test_topics.py b/tests/test_topics.py index 94fb762b..44326e56 100644 --- a/tests/test_topics.py +++ b/tests/test_topics.py @@ -269,6 +269,20 @@ def test_merge_averages_embeddings_weighted_by_document_count(): assert topics[0].embedding.tolist() == pytest.approx([8.0, 4.0]) +def test_merge_keeps_topics_that_hold_no_documents(): + """A complete mapping carries zero-document topics through a merge. + + This is what `_reduce_to_n_topics` and `_auto_reduce_topics` now guarantee by + building their mapping from `topic_ids()` rather than from document assignments. + """ + topics = build_topics({0: 8, 1: 4, 2: 2}) + topics[2].nr_documents = 0 + topics.merge({0: 0, 1: 1, 2: 1}) + + assert topics.topic_ids() == [0, 1] + assert topics.frequencies() == {0: 8, 1: 4} + + def test_merge_composes_with_an_earlier_reordering(): """The cumulative mapping tracks original topics through both operations.""" topics = build_topics({-1: 5, 0: 2, 1: 8, 2: 4}) @@ -293,10 +307,6 @@ def test_merge_sums_probability_columns(): assert topics.probabilities[0].tolist() == pytest.approx([0.1, 0.8, 0.1]) -@pytest.mark.xfail( - strict=True, - reason="Bug 3: weights divide by a zero document total; fixed in unit 3", -) def test_merge_handles_topics_with_no_documents(): """Merging topics that hold no documents falls back to equal weighting.""" topics = Topics().initialize([0, 1]) @@ -308,10 +318,6 @@ def test_merge_handles_topics_with_no_documents(): assert topics[0].embedding.tolist() == pytest.approx([5.0, 10.0]) -@pytest.mark.xfail( - strict=True, - reason="Bug 2: `embedding` is unbound when the first topic has none; fixed in unit 3", -) def test_merge_handles_topics_without_embeddings(): """Merging works even when no embeddings were ever computed.""" topics = Topics().initialize([0] * 8 + [1] * 2) @@ -403,21 +409,18 @@ def test_unknown_topic_ids_map_to_themselves(): assert mapping.map(99, from_original=True) == 99 -@pytest.mark.xfail( - strict=True, - reason="Bug 10: apply raises KeyError when the new mapping omits a topic; fixed in unit 3", -) -def test_mapping_tolerates_an_incomplete_new_mapping(): - """A topic missing from the incoming mapping keeps its current ID rather than raising. +def test_mapping_rejects_an_incomplete_new_mapping(): + """Omitting a current topic is a caller bug, and is reported as one. - `_reduce_to_n_topics` builds its mapping by zipping over documents, so a topic that - holds no documents never appears in it. + `_reduce_to_n_topics` used to build its mapping by zipping over documents, so a topic + holding no documents never appeared in it and the composition died on an opaque + `KeyError`. Callers now build from `topic_ids()`; this guards that from regressing. """ mapping = TopicMapping() mapping.apply({0: 0, 1: 1, 2: 2}) - mapping.apply({0: 0, 1: 1}) - assert mapping.map(2, from_original=True) == 2 + with pytest.raises(ValueError, match=r"missing topics \[2\]"): + mapping.apply({0: 0, 1: 1}) # -------------------------------------------------------------------------------------- @@ -460,10 +463,6 @@ def test_disk_round_trip_omits_data_matrices(): assert restored[0].embedding.size == 0 -@pytest.mark.xfail( - strict=True, - reason="Bug 11: an all-zero sparse row is dropped and loses its width; fixed in unit 3", -) def test_round_trip_preserves_the_width_of_an_all_zero_c_tf_idf_row(): """A topic whose c-TF-IDF is entirely zero keeps its column count. @@ -480,10 +479,6 @@ def test_round_trip_preserves_the_width_of_an_all_zero_c_tf_idf_row(): assert restored.c_tf_idf.shape == (2, 2) -@pytest.mark.xfail( - strict=True, - reason="Bug 8: TopicHierarchy.to_dict omits full=True, dropping node data; fixed in unit 3", -) def test_hierarchy_round_trip_preserves_node_data(): """Hierarchy nodes keep their embeddings and c-TF-IDF through serialisation.""" hierarchy = TopicHierarchy(n_leaves=1)