Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes issue #53) #55

qpwo · 2019-06-13T20:28:15Z

Initial draft. I forgot some things in PR 54 that @ConstantineLignos pointed out; should I fix those in this PR?

berquist · 2019-06-13T20:31:33Z

codecov · 2019-06-14T15:08:21Z

Codecov Report

Merging #55 into master will increase coverage by 0.1%.
The diff coverage is 94.73%.

@@            Coverage Diff            @@
##           master      #55     +/-   ##
=========================================
+ Coverage   90.66%   90.76%   +0.1%     
=========================================
  Files           6        6             
  Lines         621      639     +18     
=========================================
+ Hits          563      580     +17     
- Misses         58       59      +1

Impacted Files	Coverage Δ
immutablecollections/_immutableset.py	`90.47% <100%> (+0.12%)`	⬆️
immutablecollections/_immutabledict.py	`87.8% <93.33%> (+0.76%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1daaa9...14080a9. Read the comment docs.

qpwo · 2019-06-19T14:32:55Z

Do we want immutablesetmultidict_from_unique_keys and immutablelistmultidict_from_unique_keys and associated kwargs?

berquist · 2019-06-19T14:35:17Z

Probably not, because the point of the multidicts is to associate a single key with multiple values. A user would fully expect multiple keys to appear.

qpwo · 2019-06-19T14:37:01Z

Oh I think this might be ready to merge then

berquist

Did you already make tests for ImmutableSet?

immutablecollections/_immutabledict.py

berquist · 2019-06-19T14:49:34Z

immutablecollections/_immutableset.py

@@ -100,7 +100,8 @@ def immutableset(
            iteration_order.append(value)
        elif forbid_duplicate_elements:
            raise ValueError(
-                "Input collection has duplicate items and forbid_duplicate_elements=False"
+                "Input collection to `immutableset` contains "
+                "{!r} twice and forbid_duplicate_elements=True".format(value)


Like the dict case, print all the duplicate elements, not just the first one you find.

Co-Authored-By: Eric Berquist <eric.berquist@gmail.com>

qpwo · 2019-06-19T14:59:49Z

Did you already make tests for ImmutableSet?

Yes that was in PR 54

ConstantineLignos

Looks good overall, one small thing.

immutablecollections/_immutabledict.py

gabbard · 2019-06-20T14:15:19Z

These are the coding standards: https://github.com/isi-nlp/isi-flexnlp/blob/master/docs/coding_standards.rst

I don't see the "boolean arguments should always be keywords" rule there, but I remember discussing it. @berquist , can you add it?

ConstantineLignos · 2019-06-20T14:52:42Z

@rgabbard Discussion was here, btw: https://github.com/isi-nlp/isi-flexnlp/issues/266#issuecomment-360886337 . (You can see me being quite confused about how this works in Python in that discussion :).)

gabbard · 2019-06-20T15:28:54Z

immutablecollections/_immutabledict.py

+        keys: List[KT]
+        # `for x in dict` grabs keys, but `for x in pairs` grabs pairs, so we must branch
+        if isinstance(iterable, Mapping):
+            # duplicate keys are possible if input is e.g. a multidict


Mapping guarantees/requires that .keys return a set (and our MultiDicts don't implement Mapping)

What types should we check for then? dict_keys has some set behavior but fails isinstance:

In [1]: d = {i:i*2 for i in range(10000)} In [2]: type(d.keys()) Out[2]: dict_keys In [3]: isinstance(d.keys(), set) Out[3]: False In [4]: from typing import Mapping In [5]: isinstance(d, Mapping) Out[5]: True In [6]: from typing import Set In [7]: isinstance(d.keys(), Set) Out[7]: False

We could branch on whether iterable has .keys, or we could have a weaker debug message that just says some unknown duplicate keys were present

The code is actually fine; it's just the comment which is wrong :-)

@qpwo : We do want to include the duplicate elements. But we want to keep the "happy path" (which will be by far the most common) very fast; once we know there is an error, then we can do the work to figure out what the duplicate elements are (because no one cares if their crash takes an extra millisecond longer :-) )

gabbard · 2019-06-20T15:35:41Z

immutablecollections/_immutabledict.py

+) -> "ImmutableDict[KT, VT]":
+    """
+    Create an immutable dictionary with the given mappings, but raise ValueError if
+    *iterable* contains the same item twice. More information in `immutabledict`


The docstring here is not quite accurate - we throw on a duplicate key, not a duplicate item

gabbard · 2019-06-20T15:36:42Z

immutablecollections/_immutableset.py

@@ -92,16 +92,21 @@ def immutableset(
                "iteration order, specify disable_order_check=True"
            )

+    duplicated = set()  # only used if forbid_duplicate_elements=True


No reason to spend time allocating this unless forbid_duplicate_elements is True

gabbard · 2019-06-20T15:37:10Z

immutablecollections/_immutableset.py

+    if forbid_duplicate_elements and duplicated:
+        raise ValueError(
+            "forbid_duplicate_elements=True, but some elements "
+            "occur multiple times in input: {}".format(duplicated)


Prefer f-strings to .format

gabbard · 2019-06-20T16:04:20Z

immutablecollections/_immutabledict.py

+            keys = [key for key, value in iterable]
+        seen: Set[KT] = set()
+        duplicated: Set[KT] = set()
+        for key in keys:


So, this is really super-slow compared to the call to dict() hidden inside _RegularDictBackedImmutableDict (which is implemented in C). Instead, I'd suggest doing a two-part approach:

if forbid_duplicate_keys is True, then list-ify the input Iterable.

after we construct ret, then if forbid_duplicate_keys is True, compare then len of the ret to the len of the input iterable. If there is a mismatch, then do the work to construct the error message

Added immutabledict duplicate blocking (Start of code for issue #53)

b17b722

qpwo changed the title ~~Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes #53)~~ Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes issue #53) Jun 13, 2019

qpwo added 2 commits June 14, 2019 10:46

Fixed immutableset duplicate item error message

b3c3ac5

auto formatting and proper type management

71f214f

qpwo requested review from berquist and ConstantineLignos June 19, 2019 14:36

berquist requested changes Jun 19, 2019

View reviewed changes

better error message

972754d

Co-Authored-By: Eric Berquist <eric.berquist@gmail.com>

qpwo added 3 commits June 19, 2019 11:39

improved tests

7757f50

made immutableset print all duplicate items

6b56310

made immutabledict print ALL the duplicate keys on fail

8c71b5d

qpwo requested a review from berquist June 19, 2019 18:32

ConstantineLignos suggested changes Jun 19, 2019

View reviewed changes

immutablecollections/_immutabledict.py Outdated Show resolved Hide resolved

Made forbid_duplicate_keys keyword only

14080a9

ConstantineLignos approved these changes Jun 19, 2019

View reviewed changes

berquist approved these changes Jun 19, 2019

View reviewed changes

berquist merged commit 9127fc2 into master Jun 20, 2019

berquist deleted the 53-dicts-warn-on-duplication branch June 20, 2019 15:24

gabbard suggested changes Jun 20, 2019

View reviewed changes

qpwo mentioned this pull request Jun 20, 2019

speed up immutableset and immutabledict #56

Merged

berquist mentioned this pull request Oct 1, 2019

Provide ImmutableMultiDict factory methods which ban duplicate additions #63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes issue #53) #55

Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes issue #53) #55

qpwo commented Jun 13, 2019 •

edited

Loading

berquist commented Jun 13, 2019

codecov bot commented Jun 14, 2019 •

edited

Loading

qpwo commented Jun 19, 2019

berquist commented Jun 19, 2019

qpwo commented Jun 19, 2019

berquist left a comment

berquist Jun 19, 2019

qpwo commented Jun 19, 2019

ConstantineLignos left a comment

gabbard commented Jun 20, 2019

ConstantineLignos commented Jun 20, 2019

gabbard Jun 20, 2019

qpwo Jun 20, 2019

qpwo Jun 20, 2019

gabbard Jun 20, 2019

gabbard Jun 20, 2019

gabbard Jun 20, 2019

gabbard Jun 20, 2019

gabbard Jun 20, 2019

gabbard Jun 20, 2019

Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes issue #53) #55

Provide ImmutableDict & ImmutableMultiDict factory methods which ban duplicate additions (Closes issue #53) #55

Conversation

qpwo commented Jun 13, 2019 • edited Loading

berquist commented Jun 13, 2019

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

qpwo commented Jun 19, 2019

berquist commented Jun 19, 2019

qpwo commented Jun 19, 2019

berquist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qpwo commented Jun 19, 2019

ConstantineLignos left a comment

Choose a reason for hiding this comment

gabbard commented Jun 20, 2019

ConstantineLignos commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qpwo commented Jun 13, 2019 •

edited

Loading

codecov bot commented Jun 14, 2019 •

edited

Loading