-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
remove duplicate checks to increase performance and add functionality #37
Comments
I'm not sure what you mean by "therefore allowing that same functionality as a regular map", given that the inverse of a bidict already implements the Mapping interface: >>> from collections import Mapping
>>> b = bidict(H='hydrogen')
>>> isinstance(b.inv, Mapping)
True Maybe it would help if you gave an example of the code you'd like to be able to write, and an explanation of how the current implementation is hindering you. In any case, be aware that under the hood, a bidict and its inverse are both just wrappers around the same two backing dicts, with just the pointers to the forward and the inverse dicts swapped. Also note that
If you skip the duplication checks, you end up with an incorrect implementation that suffers from bugs like this:
Did you read any of the pertinent documentation? https://bidict.readthedocs.io/basic-usage.html#uniqueness-of-values and https://bidict.readthedocs.io/addendum.html#performance include relevant rationale about safety and performance. Thanks for your interest in bidict and for taking the time to propose improvements you're interested in. At this point I don't think there's anything here but you're more than welcome to clarify. |
Thanks for your kind reply! What I meant by "therefore allowing that same functionality as a regular map" is that it is advantageous, even expected, to allow different keys to have the same value. I'm not proposing simply removing the checks. I'm thinking you could store the inverse dict's values in a list or set, something like: from collections import defaultdict
class BidirectionalMapping(Mapping):
def __init__(self, *args, **kw):
...
self._inv = defaultdict(list) # Could be set instead, but list can maintain
# insert order which may be handy
...
class bidict(BidirectionalMapping, MutableMapping):
def __delitem__(self, key):
del self._fwd[key]
inv = self._inv # save dot-lookup time in more common no duplicate case
inv[val].remove(key)
if not inv[val]:
del inv[val]
def __setitem__(self, key, val):
self._fwd[key] = val
self._inv[val].append(key) # Maintains insert order |
Citation needed :) Here are some counterexamples:
((( I'm going to close this as it still seems pretty off the mark, but I'm grateful for your interest in bidict and hope you find it useful! |
@Ofekmeister I just came across @mahmoud's https://boltons.readthedocs.io/en/latest/dictutils.html#boltons.dictutils.OrderedMultiDict.inverted Wish I'd realized this sooner so I could have referred you to it right off the bat. For future reference, what you're talking about (where the same key can be associated with multiple values) is called a "multidict", and is commonly seen in http-related or -inspired libraries, e.g. [1,2]. bidict is intended to address a different set of use cases from the ones that multidicts address. But it looks like you can use boltons to get what you're looking for. Hope this helps! [1] https://github.com/aio-libs/multidict/ |
Would a pull request be considered that would make the inverse dict a defaultdict(list) therefore allowing that same functionality as a regular map? Removing the duplicate checks would also speed up bidict.
The text was updated successfully, but these errors were encountered: