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

add normalisation to confidence scores #4902

Merged
merged 41 commits into from
Jan 23, 2020
Merged

Conversation

akelad
Copy link
Contributor

@akelad akelad commented Dec 4, 2019

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@akelad
Copy link
Contributor Author

akelad commented Dec 4, 2019

@Ghostvv started a PR on this, few questions before I polish it up and request a review:

  • I would lean towards making the default be True, what do you think? though then i don't think we can put it in a minor :D
  • should we use a lower number than 10 for the scores? i know we discussed 5 as well

@Ghostvv
Copy link
Contributor

Ghostvv commented Dec 4, 2019

I would use LABEL_RANKING_LENGTH as normalization length, otherwise it is not clear how to report other scores in there.
True by default - yes. Not sure about the version, it is not even model breaking. But it has a danger to break whole bot functionality.
Did you test, that the scores become more intuitive after normalization?

@akelad
Copy link
Contributor Author

akelad commented Dec 4, 2019

I would use LABEL_RANKING_LENGTH as normalization length, otherwise it is not clear how to report other scores in there.

as zero 😂

No i haven't tested yet, i still need to do that. just wanted to get what i had done on a PR first

@tmbo
Copy link
Member

tmbo commented Dec 10, 2019

I don't think this shouldn't go into a patch release, please change the base to master. Next minor is next week, so that would be last chance this year

@tmbo tmbo added this to the Rasa 1.6 milestone Dec 10, 2019
@Ghostvv
Copy link
Contributor

Ghostvv commented Dec 11, 2019

@akelad could you please add normalization to EmbeddingPolicy as well?

@erohmensing erohmensing changed the base branch from 1.5.x to master December 13, 2019 03:28
@erohmensing
Copy link
Contributor

Screenshot 2019-12-12 at 22 31 27

Histogram for sara on a held out test set with no normalization, normalizaton of the top 10 intents, and normalization of the top 5 intents, in that order

@erohmensing
Copy link
Contributor

erohmensing commented Dec 13, 2019

@Ghostvv I think it is just as easy to make the number configurable as to make it a boolean -- i've implemented it as such here (same for embedding policy), where 0 is no normalization. with respect to how to report the other intents, do you think it makes sense just to always report the number of intents that we have normalized, if we normalized them (essentially doing I would use LABEL_RANKING_LENGTH as normalization length,, but in reverse )? that's also how i went about it here.

Missed that you wanted to have it default:true, but figured i can wait to change that once we decide what the default number should be. Included histograms for sara, scores definitely become more intuitive with some level or normalization, just don't know how far to go with it

@wochinge
Copy link
Contributor

@Ghostvv @erohmensing What's the state with this? Are you still planning to get this into Rasa 1.6?

Copy link
Contributor

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to introduce it to response selector as well. Please add docs

rasa/nlu/classifiers/__init__.py Outdated Show resolved Hide resolved
rasa/core/policies/embedding_policy.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
@erohmensing
Copy link
Contributor

Everything should be implemented -- could use a few tests, let me know if you have any idea how to go about those (could count the number of non-zero actions and the length of the intent output)? Also feel free to change how I've explained it because 🤷‍♂

Would really like to get this on the train 🚋

Copy link
Contributor

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please add tests, we could create a couple of classifiers/policies, where we set different ranking_length (including 0) and check that the output is how we like it

rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
rasa/core/policies/embedding_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/embedding_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/embedding_policy.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
@Ghostvv
Copy link
Contributor

Ghostvv commented Dec 17, 2019

sorry, accidentally reviewed old commit, but comments are still valid

@wochinge
Copy link
Contributor

@erohmensing @Ghostvv I'm sorry, but this missed the release train.

@tmbo tmbo removed this from the Rasa 1.6 milestone Dec 19, 2019
rasa/core/policies/embedding_policy.py Outdated Show resolved Hide resolved
rasa/nlu/classifiers/embedding_intent_classifier.py Outdated Show resolved Hide resolved
rasa/utils/train_utils.py Outdated Show resolved Hide resolved
tests/core/test_policies.py Show resolved Hide resolved
@akelad akelad added this to the Rasa 1.7 milestone Jan 23, 2020
@Ghostvv Ghostvv merged commit 13d98a9 into master Jan 23, 2020
@Ghostvv Ghostvv deleted the add_softmax_normalisation branch January 23, 2020 14:47
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.

5 participants