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

Feature top n intent #623 #715

Closed
wants to merge 9 commits into from

Conversation

dipanjannag
Copy link

@dipanjannag dipanjannag commented Dec 4, 2018

Description:

"intent_classifier_config": {
  "unit_name": "log_reg_intent_classifier",
    "data_augmentation_config": {
      "min_utterances": 20,
      "noise_factor": 5,
      "add_builtin_entities_examples": True,
      "max_unknown_words": 0,
      "unknown_word_prob": 0,
      "unknown_words_replacement_string": None
      "return_top_n_intents" : True,
      "top_n_intents_count": 3
}

Checklist:

  • My PR is ready for code review
  • I have added some tests, if applicable, and run the whole test suite, including linting tests
  • I have updated the documentation, if applicable

@codecov-io
Copy link

codecov-io commented Dec 4, 2018

Codecov Report

Merging #715 into develop will decrease coverage by 0.04%.
The diff coverage is 82.6%.

@@             Coverage Diff             @@
##           develop     #715      +/-   ##
===========================================
- Coverage       88%   87.95%   -0.05%     
===========================================
  Files           65       65              
  Lines         3809     3843      +34     
  Branches       721      733      +12     
===========================================
+ Hits          3352     3380      +28     
- Misses         348      350       +2     
- Partials       109      113       +4

@adrienball
Copy link
Contributor

Hi @deeiip ,
Thanks for this PR. Actually, adding an API to return the top intents is part of our current roadmap. However, we are still discussing how to handle this properly but we already agreed on the fact that it should rather take the form of an updated API than something in the configuration.
What we are considering at the moment would be to add an optional parameter to the parse API:

nlu_engine = SnipsNLUEngine().fit(dataset)
single_result = nlu_engine.parse("hello world")
top_results = nlu_engine.parse("hello world", top_intents=3)
assert(isinstance(single_result, dict))
assert(isinstance(top_results, list))

Another way to do it would be to add a new API:

nlu_engine = SnipsNLUEngine().fit(dataset)
top_results = nlu_engine.parse_top_intents("hello world", 3)

This solution is a bit less pythonic though.

I will keep you updated on how we decide to move forward.
Best
Adrien

@adrienball adrienball mentioned this pull request Dec 11, 2018
3 tasks
@adrienball
Copy link
Contributor

Hey @deeiip,
We have worked on several new parsing APIs in #724.
This probably covers the feature brought by your PR.
Just wanted to let you know about it.
Cheers

@dipanjannag
Copy link
Author

@adrienball thanks for adding the feature. This feature should help a lot in our product. And obviously we don't need to take the headache of maintaining our own modified copy of snips.

@adrienball
Copy link
Contributor

Hey @deeiip ,
This API is now available in the 0.19.0 release. You can check the release notes and the corresponding documentation.
Thanks!

@adrienball adrienball closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants