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

embedding layer #1401

Merged
merged 2 commits into from May 21, 2018

Conversation

Projects
None yet
3 participants
@haritha1313
Contributor

haritha1313 commented May 20, 2018

Just added an alias for the existing lookup layer, to use it as embedding layer.

@zoq

zoq approved these changes May 20, 2018

This looks good to me; According to our merge policy, I'll let this sit for 1 day before I'll merge this in, to give anyone else a chance to take a look at the code.

@rcurtin

Looks good to me. Should we use a template typedef so that the Embedding layer can work with other MatTypes?

@haritha1313

This comment has been minimized.

Contributor

haritha1313 commented May 21, 2018

Sure, I'll add it. I think as of now my work will need only arma::mat for embedding.

@zoq zoq merged commit f025772 into mlpack:master May 21, 2018

1 of 5 checks passed

Memory Checks Build started sha1 is merged.
Details
Static Code Analysis Checks Build started sha1 is merged.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Style Checks Build finished.
Details
@zoq

This comment has been minimized.

Member

zoq commented May 21, 2018

Thanks again for the contribution, should make the NCF code cleaner.

@haritha1313 haritha1313 deleted the haritha1313:embedding branch May 22, 2018

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