-
Notifications
You must be signed in to change notification settings - Fork 155
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 "excludes" to Intent #156
base: master
Are you sure you want to change the base?
Conversation
Excluding one or more entities will cause a match to fail if any of those entities are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting off with a request for some additional documentation and perhaps some more tests.
I have some concerns about how unrelated intents may interact with each other, especially with regex entities (that have a high likelihood of over-matching and unintentionally suppressing unrelated intents).
I think this is less of an issue with the DomainIntentDeterminationEngine
, but definitely a problem for uses of IntentDeterminationEngine
. I was hoping that the fixes to scoring on the push to 1.0 would allow us to deprecate DomainIntentDeterminationEngine
, but this make it more difficult. I think I'd like to see tests for exclusions across domains (to ensure they don't interact)
Out of band of this PR, we'll want to update the adapt docs on gitbook as well.
I would also like to see an addition to some the examples in the examples
directory to demonstrate usage.
I am honestly very excited to see a new contributor!
Hope this one isn't forgotten, still looking forward for it :) |
fingers crossed this one was closed accidentally 🤞 |
I haven't had the time to get this over the finish line 🫤 |
@JarbasAl @goldyfruit Can you link some examples of this being used to confirm things function as expected? I don't see any unit test coverage so I'd like to see some examples of how its working |
Some tests coverage are already in the PR 🥇 |
Thanks @NeonDaniel 👍 |
Looking at the automation, this may also need a version bump in the PR.. I also see tests should have been triggered on PR so I'm not sure why that didn't happen |
Excited to see this getting some traction again! While the test added provides coverage for the full integration of I think I've reasoned my way around how this will interact in a system with many intents (it should be relatively isolated to itself, though with the expected vocabulary comingling). Also, +1 to @NeonDaniel 's note that we need a version bump here. |
Excluding one or more entities will cause a match to fail if any of those entities are present.