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 entity synonym fuzzy match #1629

Closed
wants to merge 19 commits into from

Conversation

odannyc
Copy link

@odannyc odannyc commented Jan 18, 2019

Proposed changes:

  • Add fuzzy matching to ner_synonyms

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

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2019

CLA assistant check
All committers have signed the CLA.

@akelad
Copy link
Contributor

akelad commented Jan 21, 2019

Thanks for submitting this PR, we'll give it a review as soon as possible

@MetcalfeTom
Copy link
Contributor

Linking this to the original issue: #1626

@tmbo
Copy link
Member

tmbo commented Jan 25, 2019

We can not depend on fuzzywuzzy because as far as I can see it is released under the GPL license, which would require us to relicense NLU under GPL as well

@MetcalfeTom
Copy link
Contributor

@odannyc Any chance you could get this working with a library under an Apache 2.0 license?

@odannyc
Copy link
Author

odannyc commented Jan 30, 2019

@tmbo @MetcalfeTom

Before I dive too deep into implementing another library, I found this which has the MIT license.

Is that fine?

@odannyc
Copy link
Author

odannyc commented Feb 2, 2019

@MetcalfeTom

I have updated this PR to use a package that doesn't have the GPL license.
Let me know if it looks good now.

Thanks!

@odannyc
Copy link
Author

odannyc commented Feb 6, 2019

@akelad @MetcalfeTom

Still waiting on a response for this.

Thanks!

@akelad
Copy link
Contributor

akelad commented Feb 7, 2019

@odannyc we appreciate your hard work, but this requires some further discussion from our side. We'll get back to you once we've discussed it and ran some experiments ourselves.

@codeclimate
Copy link

codeclimate bot commented Feb 12, 2019

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

View more on Code Climate.

@odannyc
Copy link
Author

odannyc commented Feb 20, 2019

@akelad

Any news?

@akelad
Copy link
Contributor

akelad commented Feb 20, 2019

not yet sorry, we haven't had time to properly look into it yet

@tmbo
Copy link
Member

tmbo commented Mar 6, 2019

What we still need to validate: earlier tests showed that fuzzy matching is quite slow. To avoid introducing a component that can't be used in production, we need to validate whether we can really on the chosen matching library to deliver good performance.

@tmbo tmbo added type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. and removed requires disussion labels Jun 28, 2019
@tmbo
Copy link
Member

tmbo commented Aug 2, 2019

I'll close this for now as it is quite outdated. But happy to reopen if @Ghostvv thinks this makes sense from a entity matching accuricy standpoint.

@tmbo tmbo closed this Aug 2, 2019
taytzehao pushed a commit to taytzehao/rasa that referenced this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants