Skip to content
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

mention_extractor.apply with clear=True fails if it's not the first run #424

Merged
merged 3 commits into from
May 26, 2020

Conversation

HiromuHota
Copy link
Contributor

@HiromuHota HiromuHota commented May 20, 2020

I think this is a regression caused by #381 .
More specifically, #381 removed fonduer.utils.utils.get_dict_of_stable_id, which retrieves contexts that are created previously.

MentionExtractorUDF.apply checks if a temporary context would conflict with existing contexts. See below:

# Re-use a persisted context if exists.
if stable_id in dict_of_stable_id:
context = dict_of_stable_id[stable_id]
# Persist a temporary context.
else:
context_type = child_context._get_table()
context = context_type(child_context)
dict_of_stable_id[stable_id] = context

Because dict_of_stable_id is initialized with an empty dict (ie dict_of_stable_id = {}), it fails to check a temporary context against existing contexts.
Before #381, dict_of_stable_id is populated as dict_of_stable_id = get_dict_of_stable_id(doc).

@HiromuHota HiromuHota marked this pull request as ready for review May 23, 2020 02:52
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2020

Codecov Report

Merging #424 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   82.41%   82.47%   +0.05%     
==========================================
  Files          86       86              
  Lines        4361     4364       +3     
  Branches      808      809       +1     
==========================================
+ Hits         3594     3599       +5     
+ Misses        576      575       -1     
+ Partials      191      190       -1     
Flag Coverage Δ
#unittests 82.47% <100.00%> (+0.05%) ⬆️
Impacted Files Coverage Δ
src/fonduer/candidates/mentions.py 88.38% <100.00%> (+1.07%) ⬆️
src/fonduer/utils/utils.py 79.31% <100.00%> (+1.53%) ⬆️

@senwu senwu requested a review from lukehsiao May 26, 2020 03:18
@senwu
Copy link
Collaborator

senwu commented May 26, 2020

Can we test this patch with image tutorial? https://github.com/HazyResearch/fonduer-tutorials/tree/master/hardware_image

@HiromuHota
Copy link
Contributor Author

Thanks you for your feedback.
I tested the patch against all the tutorials and passed.
https://github.com/HiromuHota/fonduer-tutorials/runs/708125397

Copy link
Contributor

@lukehsiao lukehsiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lukehsiao lukehsiao merged commit af6947b into HazyResearch:master May 26, 2020
@lukehsiao lukehsiao added this to the v0.8.3 milestone May 26, 2020
@lukehsiao lukehsiao added the bug Something isn't working label May 26, 2020
@HiromuHota HiromuHota deleted the fix/regression_from_381 branch May 26, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants