compare namespace in counter and min_count

[nadgeri14]

fix for #2292

[nadgeri14]

@matt-gardner Can you please review the PR?
Thanks :)

[matt-gardner]

Two issues:

1. There are some failing tests - can you look at the build log and figure out what tests are failing, and why, and fix them?
2. Can you add a test to vocabulary_test.py that will make sure this implementation is correct? The logic is simple enough, but a test like that makes sure that we don't have regressions later.

[nadgeri14]

@matt-gardner The build error is while executing:
Unit Tests (pytest) in allennlp/common/testing/model_test_case.py:33

If I am not mistaken for example if we have

counter = defaultdict(<function Vocabulary.from_instances.<locals>.<lambda> at 0x7f3cd59e0510>, {'source_tokens': defaultdict(<c...: 1, 'secret': 1, 'sneaky': 1, 'retain': 1, 'composure': 1, 'ecstatic': 1, 'not': 1, 'disruption': 1, 'bursting': 1})})

min_count = {'tokens': 1}
It should throw an error saying tokens is not present in counter, is my assmuption right?

[matt-gardner]

Yes, that's what's happening. You need to figure out what's wrong in the code that's causing that error, then fix it, and put the fix in this PR. Probably easiest if you run the failing test locally.

[brendan-ai2]

@nadgeri14, it would be great to have this fix in if you still have the time to work on it!

[nadgeri14]

@brendan-ai2 @matt-gardner Hey sorry for lately being inactive, I have exams, as a result, I could not attend it. I will try to close the PR by the end of next week.

[schmmd]

@nadgeri14 no worries--thanks for the contribution!

[schmmd]

@nadgeri14 should we leave this open? Do you still expect to get to this?

[dirkgr]

Just a minor comment on the changelog. Feel free to merge!

[dirkgr]

Just change the wording here. It no longer "should". Now it "does"!