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

Input dropout #5636

Merged
merged 12 commits into from
Apr 21, 2020
Merged

Input dropout #5636

merged 12 commits into from
Apr 21, 2020

Conversation

Ghostvv
Copy link
Contributor

@Ghostvv Ghostvv commented Apr 15, 2020

Proposed changes:

  • add dropout before ffnn

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)

@Ghostvv Ghostvv requested a review from tabergma April 15, 2020 13:38
@Ghostvv Ghostvv changed the base branch from master to 1.9.x April 15, 2020 13:38
Copy link
Contributor

@tabergma tabergma left a comment

Choose a reason for hiding this comment

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

Let's add a changelog so that we can find this change later again.

@Ghostvv Ghostvv requested a review from dakshvar22 April 15, 2020 13:41
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Apr 16, 2020

@dakshvar22 @tabergma do we want to merge this one?

@dakshvar22
Copy link
Contributor

@Ghostvv As discussed, can you test it on a couple of datasets and configs before merging it?

changelog/5636.bugfix.rst Outdated Show resolved Hide resolved
Co-Authored-By: Daksh Varshneya <d.varshneya@rasa.com>
@tabergma
Copy link
Contributor

This also requires retraining of models, doesn't it? Should we increase the minimum model version number and add a note to the changelog entry? Or should we push it to master so that we release it with the next minor?

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Apr 17, 2020

this one doesn't because dropout is turned off during prediction so prediction graph is the same

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Apr 17, 2020

but maybe merging it into master is safer bet

@Ghostvv Ghostvv changed the base branch from 1.9.x to master April 17, 2020 08:51
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Apr 17, 2020

let's merge 1.9.x in a separate PR first: #5660

@Ghostvv Ghostvv mentioned this pull request Apr 20, 2020
4 tasks
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Apr 20, 2020

I added a parameter use_dense_input_dropout (similar to sparse one) and set it to False by default in EmbeddingIntentClassifier just to avoid surprises

Copy link
Contributor

@dakshvar22 dakshvar22 left a comment

Choose a reason for hiding this comment

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

Would require increasing the minimum compatible version right? Since we added a new layer by default, it would be a model breaking version I guess.

docs/nlu/components.rst Outdated Show resolved Hide resolved
rasa/nlu/classifiers/diet_classifier.py Show resolved Hide resolved
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Apr 21, 2020

it needs to increase anyhow, since master is not compatible with 1.9, but I don't think it requires here, since dropout doesn't affect prediction graph

@Ghostvv Ghostvv merged commit cd30622 into master Apr 21, 2020
@Ghostvv Ghostvv deleted the input-dropout branch April 21, 2020 20:16
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