-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
|
||
@classmethod | ||
def _get_pretrained_file(cls, embedding_root, pretrained_file_name): | ||
from ...gluon.utils import check_sha1, download |
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 is one minor additional change to avoid changing the mxnet import chain in __init__
8635899
to
17a9343
Compare
from .embedding import TokenEmbedding | ||
|
||
|
||
class Glossary(TokenEmbedding): |
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 this is more commonly called Vocabulary
I'll do a pass some time soon. API naming and interfaces need to be changed. |
@piiswrong I submitted this PR for respecting your request to move the text API to contrib. Given that the original PR has spanned a month, and you commented on it, I was under the impression that you had plenty of time to provide feedback on it. That said, let's address your concerns step-by-step. Since @astonzhang has iterated with me on this over quite some time in #8763, there would be lots of context to help explain the motivation behind the design, on which Aston and I are happy to fill you in. Then, if you still think that API changes need to be done, I agree with @mli that if you would propose the desired changes through an email thread on dev-list, issues, or through a new PR proposal, it would be much preferred. Besides, I look forward to learning the more desirable design from you. Thank you very much. |
Description
This is resubmission of #8763 #9394 @astonzhang
Checklist
Essentials
make lint
)Changes