-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fully merge the Watson and Embeddings classification features #709
Conversation
…d out of the provider. Move most of the REST stuff from NLU into the Feature
…re level as they can be reused
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.
@dkotter, Thanks a lot for merging this. The code LGTM and it tests well. However, I have added two minor suggestions; otherwise, everything looks good.
Also, I have noticed that when we use embeddings as a provider, it also suggests the 'Uncategorized' category as a matching term. I'm not sure if this happens in my setup only. Could you please check once? If it suggests 'Uncategorized,' then we might want to filter it from the suggestions list.
…at support post statuses
…ny that don't have a UI and any that aren't supported by the post types we support
Saw the same thing. This should now be excluded |
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.
Description of the Change
Historically (though I use that term loosely since the Embeddings functionality hasn't been around that long) we've had two classification Features:
Coming out of the work in #611, these two separate features were merged down into a single Feature, with Watson and Embeddings being two different providers for that Feature.
But we left most of the functionality in each individual Provider class instead of bringing that into the Feature class. These features are very similar but have diverged a bit over time so there wasn't an easy, clean merge that could be done.
The ultimate goal was to merge those two together and bring parity between the two and that is what this PR does. All shared functionality has been moved from the individual Provider classes into the Classification Feature class. Any functionality that was missing between the two (like the modal popup we added to Watson but never to Embeddings) has been taken care of. In theory, the Classification Feature should now function basically the same no matter the Provider that is selected.
Closes #671
Closes #699
Closes #714
How to test the Change
I would suggest fully testing the Classification Feature with both Providers. This could involve the following:
Changelog Entry
Credits
Props @dkotter
Checklist: