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

Add dimension filter to ner_spacy #1718

Merged
merged 27 commits into from Mar 1, 2019

Conversation

Projects
None yet
4 participants
@mauricedoepke
Copy link
Contributor

commented Feb 20, 2019

This PR moves entity filtering into the extractor base class and adds its functionality to the duckling and the spacy extractor.

Closes #1705

Status (please check what you already did):

  • made PR ready for code review - Don't know what you expect here
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog
@CLAassistant

This comment has been minimized.

Copy link

commented Feb 20, 2019

CLA assistant check
All committers have signed the CLA.

@codeclimate

This comment has been minimized.

Copy link

commented Feb 20, 2019

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

View more on Code Climate.

@akelad

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2019

@mauricedoepke thanks a lot for this PR! we'll give it a review as soon as possible.
As for the "made PR ready for code review" box -- we just expect you to tick it when you want us to review it :D sometimes a PR may be a work in progress.

@tmbo tmbo requested a review from erohmensing Feb 25, 2019

@erohmensing
Copy link
Member

left a comment

This is looking good, and I really appreciate that you thought to add a test for duckling dimension filtering too. I've added some comments and a few changes, check them out :)

Show resolved Hide resolved rasa_nlu/extractors/duckling_http_extractor.py
Show resolved Hide resolved tests/base/test_extractors.py
Show resolved Hide resolved tests/base/test_extractors.py Outdated
Show resolved Hide resolved tests/base/test_extractors.py Outdated
Show resolved Hide resolved docs/components.rst Outdated
Show resolved Hide resolved tests/base/test_extractors.py Outdated

mauricedoepke and others added some commits Feb 28, 2019

@erohmensing

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@mauricedoepke the master branch got a ton of changes, make sure to pull them before changing anything -- some of my suggestions are out of date now (mostly for the tests), i'll test new ones and then edit

@erohmensing erohmensing self-requested a review Feb 28, 2019

@erohmensing
Copy link
Member

left a comment

Updated comments after master merge (above and below)

Show resolved Hide resolved CHANGELOG.rst Outdated
Show resolved Hide resolved tests/base/test_extractors.py Outdated

mauricedoepke and others added some commits Mar 1, 2019

Add component builder to ner_spacy test
Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>
Rephrased Changelog entry
Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>
Create SpacyEntityExtractor with the component builder
The entity extractors are created with the component builder during normal use. So, creating it with the component builder makes the test mimic normal use better.

Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>
Create the entity extractor with the component buider
Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>

mauricedoepke added some commits Mar 1, 2019

@mauricedoepke

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@erohmensing I think I covered all your change requests. Let me know if anything is missing.

@erohmensing
Copy link
Member

left a comment

Awesome! Thank you for rewriting those tests. Just a few tiny things and we're good to go! 🎉

Show resolved Hide resolved tests/base/test_extractors.py Outdated
Show resolved Hide resolved tests/base/test_extractors.py Outdated
Show resolved Hide resolved tests/base/test_extractors.py Outdated

erohmensing and others added some commits Mar 1, 2019

Remove Timezone from non time related test
Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>
More concise test name
Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>
More precise test name
Co-Authored-By: mauricedoepke <mauricepdoepke@gmail.com>
@mauricedoepke

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Done.

@erohmensing

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

All good to merge 👍Thanks for your contribution!

@erohmensing erohmensing merged commit 351fa3d into RasaHQ:master Mar 1, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details

tmbo pushed a commit that referenced this pull request Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.