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

Random seed split #4842

Merged
merged 7 commits into from
Dec 10, 2019
Merged

Conversation

joaorobson
Copy link
Contributor

@joaorobson joaorobson commented Nov 26, 2019

Proposed changes:

Expected behavior:

  • When a user is splitting the NLU data, a new argument specifying a random seed (in sklearn, known as random_state) could be passed, as shown below:
    rasa data split nlu --random-seed=42

Status (please check what you already did):

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

@erohmensing
Copy link
Contributor

@joaorobson is this still a draft PR or is it ready for review?

@joaorobson joaorobson force-pushed the random_seed_split branch 2 times, most recently from 5c91e2b to 6bc5d3e Compare December 7, 2019 01:09
@joaorobson joaorobson marked this pull request as ready for review December 7, 2019 01:09
@joaorobson
Copy link
Contributor Author

@joaorobson is this still a draft PR or is it ready for review?

It is ready now. I just needed to update the docs and the changelog.

@erohmensing
Copy link
Contributor

erohmensing commented Dec 9, 2019

Ok, we'll give it a review as soon as possible.

@erohmensing erohmensing requested a review from Ghostvv December 9, 2019 14:57
@Ghostvv Ghostvv requested review from tabergma and removed request for Ghostvv December 10, 2019 10:44
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.

Looks great 🚀 Just a few comments. Thanks for adding a test!

Please, update your branch and update the changelog entry (https://github.com/RasaHQ/rasa/tree/master/changelog).

@joaorobson
Copy link
Contributor Author

Looks great Just a few comments. Thanks for adding a test!

Please, update your branch and update the changelog entry (https://github.com/RasaHQ/rasa/tree/master/changelog).

Thanks. I guess that I've fixed everything that you requested.

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.

Thanks for the contribution! 🎉

@tabergma tabergma merged commit 222ce20 into RasaHQ:master Dec 10, 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.

Add set.seed ability to CLI when splitting NLU data
3 participants