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

Option to give an existing vocab file(.pt or text) to preprocess.py #1346

Merged
merged 8 commits into from
Mar 8, 2019

Conversation

kvthr
Copy link
Contributor

@kvthr kvthr commented Mar 8, 2019

Addresses the issue #1304. Tried to implement the first feature, ability to reuse the existing vocabulary.

@vince62s
Copy link
Member

vince62s commented Mar 8, 2019

Sorry, I was not clear enough in what we wanted.
The logic is good but must be placed here: https://github.com/OpenNMT/OpenNMT-py/blob/master/onmt/inputters/inputter.py#L339
rather than in preprocess.
can you adjust ?

@kvthr
Copy link
Contributor Author

kvthr commented Mar 8, 2019

Yes! Will do it!

onmt/opts.py Outdated Show resolved Hide resolved
preprocess.py Outdated Show resolved Hide resolved
onmt/inputters/inputter.py Outdated Show resolved Hide resolved
onmt/inputters/inputter.py Outdated Show resolved Hide resolved
@vince62s
Copy link
Member

vince62s commented Mar 8, 2019

looks good, thanks !

@vince62s vince62s merged commit 25547d8 into OpenNMT:master Mar 8, 2019
@guillaumekln
Copy link
Contributor

What about the target vocabulary?

@guillaumekln
Copy link
Contributor

Oh ok, they are bundled in a single .pt file.

@vince62s
Copy link
Member

vince62s commented Mar 8, 2019

Well the vocab .pt file contains both src vocab and tgt vocab.
It would be cumbersome to force users to specify -src_vocab file.pt and -tgt_vocab file.pt
So we need to document this.

@vince62s
Copy link
Member

vince62s commented Mar 8, 2019

@lordeddard can you please add a comment in the opt.py to be very specific ?
thanks.

@kvthr
Copy link
Contributor Author

kvthr commented Mar 8, 2019

Yes! Will do!

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