-
Notifications
You must be signed in to change notification settings - Fork 50
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
First batch of NLU signals #501
Conversation
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.
l f g
Ready for review |
These look good to me. The only thing I question is the use of Tags. None of the other signals have any tags on them, I'm not sure if those actually do anything, or if they're helpful in your model, or even if they work on signals, I assumed they would as it's probably the same schema, but I don't know. |
that's a fair point. i should probably add them |
these already have them, but none of the other signals do, if we're saying we want tags for all signals. The tags almost feel redundant to the name, which is why I think we didn't add them before. If having tags helps you lets add them to all signals, but if it's just superficial lets remove them from this pr? |
Oh I see what you're saying. Let's get them in as they could be useful indicators when observed with other signals |
Based on internal testing from @rw-access this should be good to merge. Someone review & give a 👍 |
FWIW this PR wasn't blocked before testing and isn't now, so don't let me hold you up! |
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.
No description provided.