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.

Sign up for GitHub

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

Jump to bottom

Fix overlapping entities #1604

Merged
merged 18 commits into from
Jan 21, 2019
Merged

Fix overlapping entities #1604

merged 18 commits into from
Jan 21, 2019

Conversation

EPedrotti
Copy link
Contributor

@EPedrotti EPedrotti commented Dec 31, 2018 •
edited by tmbo
Loading

Proposed changes:

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog

@EPedrotti EPedrotti requested a review from tmbo December 31, 2018 09:41
@EPedrotti
Copy link
Contributor Author

@tmbo codeclimate is complaining about a line being more than 79 characters. Shouldn't it be set to 80?

@tmbo
Copy link
Member

tmbo commented Jan 2, 2019

mhm good point, not sure - we do not have a codeclimate configuration so we rely on the defaults here atm.

@EPedrotti
Copy link
Contributor Author

@tmbo can we merge this one even if codeclimate complains about the line lenght being 80 chars?

tmbo
tmbo requested changes Jan 3, 2019
View reviewed changes
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

functionality looks good, please take a look at the style suggestions

rasa_nlu/evaluate.py Outdated Show resolved Hide resolved
src/pip-delete-this-directory.txt Outdated Show resolved Hide resolved
rasa_nlu/evaluate.py Outdated Show resolved Hide resolved
@tmbo
Copy link
Member

tmbo commented Jan 9, 2019 •
edited
Loading

Please let me know when this ready for another review (by requesting another review)

@tmbo
Copy link
Member

tmbo commented Jan 15, 2019

@EPedrotti PLEASE request a new review when this is done, otherwise please state what needs to be done to get this to another review

@EPedrotti
Copy link
Contributor Author

@tmbo I was on openshift and I didn't had to to review and to add tests

@tmbo tmbo added this to the 0.14 milestone Jan 20, 2019
@EPedrotti EPedrotti requested a review from tmbo January 21, 2019 15:03
tmbo
tmbo approved these changes Jan 21, 2019
View reviewed changes
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

looks great. minor style commetn.

please add the change to the Changelog.rst

rasa_nlu/evaluate.py Outdated
def do_extractors_support_overlap(extractors):
"""Checks if extractors support overlapping entities
"""
return extractors is None or not (
Copy link
Member

Choose a reason for hiding this comment

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

should be extractors is None or CRFEntityExtractor.name not in extractors

@codeclimate
Copy link

codeclimate bot commented Jan 21, 2019

Code Climate has analyzed commit df507cf and detected 0 issues on this pull request.

View more on Code Climate.

@EPedrotti EPedrotti merged commit cf06db2 into master Jan 21, 2019
@tmbo tmbo deleted the fix-overlapping-entities branch May 9, 2019 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmbo tmbo tmbo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14 release
Development

Successfully merging this pull request may close these issues.

Fix overlapping entities in evaluate script
2 participants
@EPedrotti @tmbo