ekzhu / SetSimilaritySearch

All-pair set similarity search on millions of sets in Python and on a laptop
Apache License 2.0
589 stars 40 forks source link

Allow input to be any iterable #8

Open jaklinger opened 4 years ago

jaklinger commented 4 years ago

Closes #7

Quoting from the issue:

Currently

https://github.com/ekzhu/SetSimilaritySearch/blob/master/SetSimilaritySearch/search.py#L27 https://github.com/ekzhu/SetSimilaritySearch/blob/master/SetSimilaritySearch/all_pairs.py#L28

state:

if not isinstance(sets, list) or len(sets) == 0:
        raise ValueError("Input parameter sets must be a non-empty list.")

I propose to change this to:

if not isinstance(sets, Iterable) or len(sets) == 0:
        raise ValueError("Input parameter sets must be a non-empty iterable.")

Which then allows inputs as tuple as well, as well as ordered key-sets. Was helpful in my use case, rather than having to create a copy of the data in list form.

ekzhu commented 4 years ago

Thanks for the PR. There is a problem though, consider a generator

from collections.abc import Iterable

def test():
    for i in range(len(100)):
        yield i

isinstance(test(), Iterable)
# True

len(test())
# Error

How about simplely add all the supported iterable types into the type check?

markopy commented 4 years ago

I don't think it's possible to even support iterables with the current code because it merely keeps a reference to the input set list and requires them to be indexable. So if your original data is in an iterator a copy is needed anyway.

Arguably it's kind of dirty to require the user of the library to provide the data as a list (and take care to not modify it while the index is in use, which is not documented anywhere!) but it does save the memory overhead of creating an internal copy.

ekzhu commented 4 years ago

Yes. That's why checking if input is iterator is not enough to prevent the error I mentioned.

This library is intended to provided an in-memory solution for similarity search. So I think it's fair to ask users to load all data into memory as a list, tuple, or ordered dict and keep them there. True it should be documented.