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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace in-place sets with tuples #53686
Conversation
Is this change warranted? While in general it is more efficient to lookup elements in a set rather than in a list or a tuple since those are scanned sequentially, one could object that for small sets the cost of the hash would be greater than the cost of scanning the tuple, and that this will probably be the case in HomeAssistant code. However, Python builds >>> import timeit
>>> def in_set(x): return x in {"aa", "bb", "cc", "dd"}
...
>>> def in_tuple(x): return x in ("aa", "bb", "cc", "dd")
...
>>> timeit.timeit(lambda: in_set("ee"))
0.09102824400179088
>>> timeit.timeit(lambda: in_tuple("ee"))
0.12370169399946462 Of course the larger the collection is the more efficient sets are compared to tuples, especially when the element is not found since the tuple must be scanned in its entirety. |
@samueltardieu You are right that This PR only modifies in-place sets. Any collection larger than a few elements should probably get its one constant and be defined as An area you didn't look at is the memory consumption. Here the from pympler import asizeof
t = ("aa", "bb", "cc", "dd")
s = {"aa", "bb", "cc", "dd"}
s2 = frozenset(t)
print(f"Tuple: {asizeof.asizeof(t)}")
print(f"Set: {asizeof.asizeof(s)}")
print(f"Frozenset: {asizeof.asizeof(s2)}")
--
Tuple: 296
Set: 440
Frozenset: 440 -- |
Quick testing with |
Also, contrary to a dict where you expect to get a value back most of the time when looking up the key, it is common to expect no hit when checking for membership in a set. For example if you look at the proposed change for I agree that in practice the performance penalty may be limited, but I frown upon not using the right tool for the job (sets) for the sake of "unifying the style" as a lookup is not the same thing as using an enumeration in a |
I have opened pylint-dev/pylint#4776 to discuss the soundness of this future pylint suggestion. I had not realized that @cdce8p was also the author of this lint introduced yesterday in pylint. I would suggest to at least wait until the situation on pylint side is cleared before proceeding. |
Closing this after discussion in pylint-dev/pylint#4776 |
Proposed change
Continuation of #53684
This time only for
sets
.Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: