Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented delete vector function based on the ID #57

Merged
merged 4 commits into from Sep 23, 2016

Conversation

erramuzpe
Copy link
Contributor

This is a PR trying to solve #42 and #27 . I've only done the one for Memory storage, since I don't know much about Redis.

I was trying to squash the previous commits and after some fails, decided to just start doing it properly (branching...).

@@ -89,3 +89,9 @@ def want_string(arg, encoding='utf-8'):
else:
rv = arg
return rv


def get_keys(engine):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


def test_delete_vector_multiple_hash(self):
dim = 5
hashes = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hashes = [UniBucket('name_hash_%d' % k) for k in range(10)]

def get_keys(self, engine):
for lshash in engine.lshashes:
bucket = engine.storage.buckets[lshash.hash_name][lshash.hash_name]
return {i[1] for i in bucket}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You return keys from the first bucket of the first hash and it makes your second test really weird. If you want to get all data stored in the engine you should union content of all buckets for all hashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aw, you're right... It was fine before. I will take a look later on, ok? Thanks for your help!

Copy link
Collaborator

@amorgun amorgun Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that:

def get_keys(self, engine):
    def get_bucket_keys(lshash):
        bucket = engine.storage.buckets[lshash.hash_name][lshash.hash_name]
        return (i for v, i in bucket)
    return set(itertools.chain.from_iterable(
        get_bucket_keys(lshash) for lshash in engine.lshashes))

edit: less hacky get_bucket_keys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fantastic!

"""
for bucket_key in self.buckets[hash_name]:
self.buckets[hash_name][bucket_key] = \
[(v, id_data) for v, id_data in self.buckets[hash_name][bucket_key] if id_data != data]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still too long. Try this:

    def delete_vector(self, hash_name, data):
        """
        Deletes vector and JSON-serializable data in bucket with specified key.
        """
        for bucket in self.buckets[hash_name].values():
            bucket[:] = [(v, data) for v, data in bucket if data != data]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Craftman work ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amorgun did you do it by your own or is there a tool that factors it automatically? Would you recommend me resources (books, videos...) for good practices? (I mean, testing, PEP8...)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erramuzpe I did it by my own.
I personally use PyCharm. It helps with simple code analysis like pep8 and Python 2/3 compatibility and also allows some simple refactorings. And it has free community version. There are plenty of other linters for Python.
I suggest reading "Writing Idiomatic Python" by Jeff Knupp first, it's an essential book for beginners.
Next you can try Python Cookbook. It's more complicated, but it covers almost everything that can be done in Python, including testing.

@amorgun
Copy link
Collaborator

amorgun commented Aug 2, 2016

Looks good to me. Let's summon @pixelogik.

@adityapatadia
Copy link
Contributor

Need to implement for redis too. We can't leave the storage without that method.

@adityapatadia
Copy link
Contributor

@adityapatadia adityapatadia merged commit 8f67e18 into pixelogik:master Sep 23, 2016
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.

None yet

3 participants