Skip to content

Handle unhashable element input to list_update and list_difference_update - #4975

Draft
nikolajmunk wants to merge 1 commit into
ManimCommunity:mainfrom
nikolajmunk:fix/list-difference-hashables
Draft

Handle unhashable element input to list_update and list_difference_update#4975
nikolajmunk wants to merge 1 commit into
ManimCommunity:mainfrom
nikolajmunk:fix/list-difference-hashables

Conversation

@nikolajmunk

Copy link
Copy Markdown
Contributor

Overview: What does this pull request change?

This addresses a small regression introduced by #4939, where an unhashable element in l1 and/or l2 would throw a TypeError. This PR changes the input types of list_update and list_difference_update to l1: Iterable[H], l2: Iterable[J] where H and J are Hashable TypeVars.

I have marked this PR as a draft because it is not 100% clear to me that disallowing unhashable input is the right thing to do. We never need to operate on unhashable input in the library, but perhaps a user has some weird use for it somewhere.
There are numerous easy ways to handle both hashable and unhashable input without requiring some kind of has_unhashable: bool flag, but all of them are going to be slower than the current implementation. I'm happy to write such an implementation or take suggestions for one, if people feel like we need it that is. Otherwise this should be good enough (though maybe I should write an overload or two for nicer type hints)!

Looking forward to any feedback.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

- Restrict input types of list_update and list_difference_update to iterables of hashables.
- Add instance check to avoid unnecessary conversion to list/set
@GniLudio

Copy link
Copy Markdown
Contributor

Optimizing hashable types while still allowing unhashable types would be nice.
Maybe with something like this?

def list_difference_update(l1: Iterable[T], l2: Iterable[T]) -> list[T]:
    if isinstance(l2, Iterator):
        l2 = tuple(l2)
    with contextlib.suppress(TypeError):
        l2 = set(l2)
    return [e for e in l1 if e not in l2]

@nikolajmunk

Copy link
Copy Markdown
Contributor Author

Yep, that works for cases when l2 has unhashables. But if, say, l1 has an unhashable element while l2 doesn't, then the list comprehension will try to look up an unhashable in a set and error out.

@GniLudio

Copy link
Copy Markdown
Contributor

Yep, that works for cases when l2 has unhashables. But if, say, l1 has an unhashable element while l2 doesn't, then the list comprehension will try to look up an unhashable in a set and error out.

Not really liking this solution:

def list_difference_update(l1, l2):
    if isinstance(l2, Iterator):
        l2 = tuple(l2)
    
    with contextlib.suppress(TypeError):
        l2_set = set(l2)
        
    def is_in_l2(e) -> bool:
        try:
            return e in l2_set
        except TypeError:
            return e in l2
    return [e for e in l1 if not is_in_l2(e)]
Here a small test
from typing import Iterator
import contextlib

def list_difference_update(l1, l2):
    if isinstance(l2, Iterator):
        l2 = tuple(l2)
    
    with contextlib.suppress(TypeError):
        l2_set = set(l2)
        
    def is_in_l2(e) -> bool:
        try:
            return e in l2_set
        except TypeError:
            return e in l2
    return [e for e in l1 if not is_in_l2(e)]
    
    
class MyClass:
    def __init__(self, name):
        self.name = name
        
    def __repr__(self):
        return self.name
        
    __hash__ = None

a = [MyClass("a"), MyClass("b"), MyClass("c")]
b = [1, 2, 3]
c = list_difference_update(a, b)
print(c)

@nikolajmunk

Copy link
Copy Markdown
Contributor Author

Not really liking this solution:

Haha yes, I have a few solutions that look similar. It's possible to do this in a nice-looking way, but all my attempts have resulted in an algorithm that is markedly slower when all input is hashable, which it will be the vast majority of the time. Catching the edge case is simply very expensive.

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.

2 participants