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

support UTF-8 start_text #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maraoz
Copy link
Contributor

@maraoz maraoz commented Mar 31, 2016

I was trying to run stuff like:

th sample.lua -checkpoint $(ls -t cv/check*.t7 | head -1) -length 200 -temperature 0.75 -start_text "El país necesita"

And kept getting a Got invalid idx error. After digging a while I found that lua doesn't work correctly with utf-8 strings 😠, so I had to add a package to solve this.

Please review. This PR might be controversial, as it adds a dependency, but I think that given that the main use-case for this lib is character-based learning, it should properly handle UTF-8 inputs both in the input files (#7) and CLI arguments (this PR).

@ostrosablin
Copy link

Oh, that solves #47 issue I have started! Many thanks! Hope this gets merged into the main project, but for now I'm cloning your fork.

@jcjohnson
Copy link
Owner

Thanks for figuring this one out! It seems like a relatively innocent dependency, so I think I'm onboard. Does this require any change to the decode_string method in the LanguageModel? Will it still work properly if we sample a non-ASCII character?

@maraoz
Copy link
Contributor Author

maraoz commented Apr 1, 2016

It does! Actually, that worked even without this PR

On Thu, Mar 31, 2016, 14:22 Justin [email protected] wrote:

Thanks for figuring this one out! It seems like a relatively innocent
dependency, so I think I'm onboard. Does this require any change to the
decode_string method in the LanguageModel? Will it still work properly if
we sample a non-ASCII character?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#52 (comment)

@drcege
Copy link

drcege commented Oct 29, 2016

Why this comment has not been merged?

Copy link

@mcr-ksh mcr-ksh left a comment

Choose a reason for hiding this comment

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

did the same. works but the Problem with "132: Got invalid idx" remains as the index is not found due to a bad preprocessing for the voc_table. However, I managed to get this fixed by #29

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.

6 participants