Skip to content

Conversation

@dhpollack
Copy link
Contributor

The EmbeddingBag module does not include a from_pretrained method like the Embedding module. I added it for consistency between the two modules.

@dhpollack
Copy link
Contributor Author

I think this failure ci failure is not related to my change but just a failure on the build machine itself.

@weiyangfb
Copy link
Contributor

changes look legit to me, wondering why not passing in args like max_norm, norm_type, mode etc to from_pretrained as well. cc @ssnl @soumith

@dhpollack
Copy link
Contributor Author

I can make those changes as well

@ssnl
Copy link
Collaborator

ssnl commented Dec 19, 2018

LGTM too, but please also add the other options @weiyangfb mentioned.

@ezyang
Copy link
Contributor

ezyang commented Dec 20, 2018

Sorry, one more nit: can we just add some smoketests for the new options? They're not smoketested at all in the test suite at the moment.

@dhpollack
Copy link
Contributor Author

@ezyang let me know if that is what you are looking for.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants