-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Adapt the training data reader and emulator for wit.ai to their latest format #7123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 🎉
@@ -0,0 +1,4 @@ | |||
Adapt the training data reader and emulator for wit.ai to their latest format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be very helpful to link to the wit.ai page where this format is documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is no place where the format is described (or I was unable to find it)
rm -r data/* | ||
mv /path/to/expressions.json data/ | ||
rm -rf data/ | ||
mv /path/to/utterances data/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but /data
won't exist given that you've rm -rf
-ed it in the line before 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this way it would create the directory and move it's content to data/
(you can treat is as renaming — we are basically renaming /path/to/utterances
directory here to data
)
rasa/nlu/emulators/wit.py
Outdated
"""Transform data to wit.ai format.""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use a complete docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree but I would say the method we're overriding (the one from NoEmulator
) needs to be documented instead. Will do that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
Proposed changes:
Status (please check what you already did):
black
(please check Readme for instructions)