dmlc / gluon-nlp

NLP made easy

https://nlp.gluon.ai/

Apache License 2.0

2.56k stars 538 forks source link

[BUGFIX] Update default alpha for sentencepiece tokenizer #1247

Closed haven-jeon closed 4 years ago

haven-jeon commented 4 years ago

Description

1239

Checklist

Essentials

[v] PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
[v] Changes are complete (i.e. I finished coding on this PR)
[v] All changes have test coverage
[v] Code is well-documented

Changes

[ ] Feature1, tests, (and when applicable, API doc)
[ ] Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

cc @dmlc/gluon-nlp-team

sxjscience commented 4 years ago

Not sure why the CI is failing. But the change looks good.

chenw23 commented 4 years ago

Please don't worry. I will look into the failure reason soon.

chenw23 commented 4 years ago

Hello @haven-jeon This failure is due to an error that is already fixed in the master branch #1236 For the gluon-nlp repo, master branch is its dev branch and v0.9.x is its release branch. Is there urgent need to change the code directly to the release branch rather on the dev branch? If it is possible to commit to master branch, would you please rebase your pull request on top of the current master branch? The failure will vanish automatically if you are using code on the master branch. Thanks!

haven-jeon commented 4 years ago

Hello @haven-jeon This failure is due to an error that is already fixed in the master branch #1236 For the gluon-nlp repo, master branch is its dev branch and v0.9.x is its release branch. Is there urgent need to change the code directly to the release branch rather on the dev branch? If it is possible to commit to master branch, would you please rebase your pull request on top of the current master branch? The failure will vanish automatically if you are using code on the master branch. Thanks!

If it is an already PR on master, it would be better to cancel the PR. :)

chenw23 commented 4 years ago

Sorry, Maybe I didn't express clearly about it. I am saying that the bug leading to the CI failure is fixed on the master branch. I am not saying the problem you are trying to address is fixed on the master branch. You can feel free to give this pull request on the master branch, where the CI is working well.