Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Updated recommenders example #13041

Merged
merged 7 commits into from
Nov 26, 2018

Conversation

ThomasDelteil
Copy link
Contributor

Description

Updated recommender system example:

  • Merged notebook 1 and 2 into a single one, simpler to follow
  • Removed negative sampling notebook, pointing to GluonNLP implementation that contains ImportanceSampling, NoiseContractiveLoss and NegativeSampling.
  • Removed manual sparse implementation, replaced with embedding layers
  • Updated DSSM model with modernized Gluon version
  • Replaced BagOfWordRandomProjection with Gluon version, though it really should only be used in very specific cases, added warning about that in the notebook and used embedding + lstm instead.

@ankkhedia
Copy link
Contributor

@ThomasDelteil Thanks for the contribution
@mxnet-label-bot [ pr-awaiting-testing, Example ]

@marcoabreu marcoabreu added Example pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 30, 2018
@ankkhedia
Copy link
Contributor

@aaronmarkham @thomelane @safrooze Could you please take a look ?

@Ishitori
Copy link
Contributor

Cannot comment on notebook - too big, so pasting comments here:
demo1-MF:
1)
ratings = nd.dot(net1.user_embedding.weight.data(ctx=mx.gpu(0)), net1.item_embedding.weight.data(ctx=mx.gpu(0)).T).asnumpy()

Please, replace mx.gpu with ctx[0], so it would work without gpu.

  1. net3.summary(user.as_in_context(ctx), item.as_in_context(ctx))
    Line fails with "TypeError: copyto does not support type <class 'list'>", because ctx is an array. Use ctx[0] instead.

  2. Same error in Visualizing embeddings - ctx used as a list - needs to be changed to ctx[0]

  3. Couldn't plot the last chart (with lines showing losses). Exception was: x and y must have same first dimension, but have shapes (15,) and (2,), and y (loss) is actually of shape (2,)

@ThomasDelteil
Copy link
Contributor Author

@Ishitori thanks, looks like my last minute changes broke everything :)
Doing one last full run and will push the changes

@ThomasDelteil
Copy link
Contributor Author

@Ishitori updated

@Ishitori
Copy link
Contributor

Ishitori commented Nov 6, 2018

Yes, now notebooks work fine. LGTM!

@ThomasDelteil
Copy link
Contributor Author

@indhub can you please merge this? Thanks!

@kalyc
Copy link
Contributor

kalyc commented Nov 13, 2018

@ThomasDelteil thanks for the contribution! Could you re-trigger CI with an empty commit?

@ThomasDelteil
Copy link
Contributor Author

@indhub @nswamy @sandeep-krishnamurthy can you please merge this? Thanks!

@kalyc
Copy link
Contributor

kalyc commented Nov 16, 2018

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed Example pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 16, 2018
@KellenSunderland KellenSunderland merged commit 8b3fa78 into apache:master Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants