Skip to content

fix: support list search terms in find_topics (#2475) - #2539

Open
kiruthick01 wants to merge 1 commit into
MaartenGr:masterfrom
kiruthick01:fix/find-topics-list-search-term
Open

fix: support list search terms in find_topics (#2475)#2539
kiruthick01 wants to merge 1 commit into
MaartenGr:masterfrom
kiruthick01:fix/find-topics-list-search-term

Conversation

@kiruthick01

Copy link
Copy Markdown

What does this PR do?

find_topics wraps search_term in a list before passing it to _extract_embeddings, so a list argument becomes a nested list. In sentence-transformers' Transformer.tokenize, an element that is neither a str nor a dict falls through to the text-pair branch:

else:
    batch1, batch2 = [], []
    for text_tuple in texts:
        batch1.append(text_tuple[0])
        batch2.append(text_tuple[1])

This has two consequences:

  • A one-element list raises IndexError: list index out of range (the reported bug).
  • A list of two or more terms does not crash. It is silently encoded as a sentence pair, and every term from the third onward is discarded. So users passing lists today can get quietly incorrect topic matches rather than an error, which seems worth fixing alongside the crash.

This PR normalizes search_term to a list of strings and averages the resulting word embeddings into a single query embedding. Passing a string is unchanged, since averaging a single embedding returns that embedding. Empty lists and non-string entries now raise an informative ValueError/TypeError instead of failing deep inside the tokenizer.

Fixes #2475
Fixes #2392

A note on the approach

In #2392 you noted that search_term is typed as str and not list[str], so I want to be upfront that supporting lists is a deliberate choice here rather than a settled one. The alternative — keeping the str-only contract and raising a clear TypeError — is a smaller change and I am happy to switch to it if you prefer; the silent-truncation path above is the part that I think should not stay as-is either way. Please treat the specific approach as open to discussion.

Tests

Three tests added to tests/test_representation/test_representations.py:

  • a one-element list returns the same result as passing the string directly (regression for this bug);
  • multiple search terms give order-independent results — this fails under the previous behaviour, since a (a, b) sentence pair is not equivalent to (b, a);
  • empty lists and non-string entries raise.

ruff check and ruff format --check both pass. I have not been able to run the full suite locally, so the new tests get their first real execution in CI.

Before submitting

`find_topics` wrapped `search_term` in a list before passing it to
`_extract_embeddings`, so a list argument became a nested list. In
sentence-transformers' `Transformer.tokenize` a non-str, non-dict element
falls through to the text-pair branch, which reads `text_tuple[0]` and
`text_tuple[1]`. A one-element list therefore raised an IndexError, while
a longer list was silently encoded as a sentence pair, ignoring every term
after the second one.

Normalize `search_term` to a list of strings and average the resulting
word embeddings into a single query embedding. Passing a string is
unchanged: averaging a single embedding returns that embedding. Empty
lists and non-string entries now raise an informative error.

Fixes MaartenGr#2475
Fixes MaartenGr#2392
@kiruthick01

Copy link
Copy Markdown
Author

The build (3.10) job is red, but I do not think these failures come from this PR:

FAILED tests/test_plotting/test_bar.py::test_barchart[reduced_topic_model] - assert 7 == 8
FAILED tests/test_plotting/test_bar.py::test_barchart_outlier[reduced_topic_model] - assert 7 == 8
FAILED tests/test_representation/test_representations.py::test_topic_reduction[10-reduced_topic_model] - IndexError: index 65 is out of bounds for axis 1 with size 7

All three come from the session-scoped reduced_topic_model fixture resolving to 7 topics rather than 8, via reduce_topics(documents, nr_topics="auto"). A few things point away from this change being the cause:

  • find_topics has no internal callers in the package, so it is not reachable from the reduction or plotting paths.
  • tests/test_plotting/ is collected before tests/test_representation/, so test_bar.py fails before any of the new tests run, and test_topic_reduction sits above them in the file as well.
  • build (3.14) passed on the same commit, and the remaining 276 tests passed, including the three new ones.

That reads to me like version-dependent nondeterminism in the nr_topics="auto" reduction on the 3.10 dependency set rather than anything here, but you would know far better than I would whether that fixture has been unstable. Happy to rebase if a re-run would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

find_topics(topic) crashes if topic is a 1-element list List index out of range in find_topics when called with a list of 1 string

1 participant