Scoring tests and fixes #131
Conversation
Resulted in new tests for `one_of`, as well as a replacement of a function for an itertools equivalent. Lots of updated and added docstrings!
This reverts commit 2d25817.
| """ | ||
| intent, tags = self.validate_with_tags(tags, confidence) | ||
| return intent | ||
|
|
||
| def validate_with_tags(self, tags, parse_weight): | ||
| def validate_with_tags(self, tags, confidence): |
There was a problem hiding this comment.
It's unfortunate, but this actually constitutes a breaking change (specifically named arguments; caught it when merging some new tests). I have kept all the previous fixes where we were overriding confidence or using the wrong one, and we have scoring tests to validate that.
There was a problem hiding this comment.
Is it worth keeping the compatibility with something like
def validate_with_tags(self, tags, confidence=None, parse_weight=None):
if parse_weight:
print("parse_weight is deprecated, use confidence instead")
confidence = confidence or parse_weight
if confidence is None:
raise TypeError('Missing required argument confidence')Probably not...
There was a problem hiding this comment.
I don't think so; the parser is so lightweight that I'm actually concerned about the perf of something like that. I suppose we could just emit on first use? But I don't think the changes from the PR that introduced parse_weight have made it too broadly into the world yet, so its uses should be minimal.
| """ | ||
| for tag in tags: | ||
| for entity in tag.get('entities'): | ||
| for v, t in entity.get('data'): | ||
| if t.lower() == entity_type.lower() and \ | ||
| (tag.get('start_token', 0) > after_index or tag.get('from_context', False)): | ||
| (tag.get('start_token', 0) > after_index or \ |
There was a problem hiding this comment.
the \ is technically not needed when in parenthesis like this
| """ | ||
| intent, tags = self.validate_with_tags(tags, confidence) | ||
| return intent | ||
|
|
||
| def validate_with_tags(self, tags, parse_weight): | ||
| def validate_with_tags(self, tags, confidence): |
There was a problem hiding this comment.
Is it worth keeping the compatibility with something like
def validate_with_tags(self, tags, confidence=None, parse_weight=None):
if parse_weight:
print("parse_weight is deprecated, use confidence instead")
confidence = confidence or parse_weight
if confidence is None:
raise TypeError('Missing required argument confidence')Probably not...
adapt/intent.py
Outdated
| @@ -165,17 +171,22 @@ def validate_with_tags(self, tags, confidence): | |||
| intent_confidence += confidence | |||
|
|
|||
| if len(self.at_least_one) > 0: | |||
| best_resolution = resolve_one_of(tags, self.at_least_one) | |||
| # we want to use a copy of the original tags here, as opposed to | |||
| # local_tags. This allows us to have overlaps between `required` | |||
There was a problem hiding this comment.
should this be "as opposed to tags" instead of ...local_tags
There was a problem hiding this comment.
Yeah, this whole comment block was supposed to go. I'll update.
|
As for commit history, I'm actually planning to squash, so the EventEmitter commits should just fall out. I'll need to add the version bump as well. |
Addresses some components of #130, including
Removing custom code for an itertools replacement
Updating some docs typos
Updating some docstrings to better reflect reality
Fixing scoring in
Intent.validate_with_tags, adding associated tests.Description of how to validate or test this PR
./run_tests.shshould capture new tests and linter.Whether you have signed a CLA (Contributor Licensing Agreement)
Signed