Skip to content
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

FEA: add RecVAE model #727

Merged
merged 88 commits into from
Mar 5, 2021
Merged

FEA: add RecVAE model #727

merged 88 commits into from
Mar 5, 2021

Conversation

Sherry-XLL
Copy link
Member

No description provided.

@2017pxy 2017pxy self-requested a review February 20, 2021 09:25
@2017pxy 2017pxy changed the title ADD: add RecVAE model FEA: add RecVAE model Feb 21, 2021
@deklanw
Copy link
Contributor

deklanw commented Feb 25, 2021

@Sherry-XLL Nice work! I have a question though:

The RecVAE repo says

Current model is slightly different from the one described in the paper, you can find original code here.

With a link to a different branch.

Which branch did you base your code on? Unfortunately the repo doesn't explain the differences explicitly. I'm curious if you figured it out while implementing it.

@Sherry-XLL
Copy link
Member Author

Sherry-XLL commented Feb 26, 2021

@deklanw First of all, thank you very much for your attention to our RecVAE code, which is based on the github link given in the paper at https://github.com/ilya-shenbin/RecVAE.

As for the current model and the original code mentioned in the RecVAE repo, there is no essential difference between them. The paper’s description of the model implementation is based on the original ipython notebook, which is convenient for debugging and outputting the paper results. In order to encapsulate the code into a complete project, the model in this repo uses three classes including Encoder, CompositePrior and VAE to modularize the source code, and this is the reason why we use the code in this repo as a reference.

@deklanw
Copy link
Contributor

deklanw commented Feb 26, 2021

@Sherry-XLL I see, thanks. So it was just refactoring a notebook into cleaner modules?

Idk how close to finished this PR is, but just looking through the code I've noticed a few things:

@Sherry-XLL
Copy link
Member Author

@deklanw

  • As for the question 1, the encoder used 5-layer densely connected convolutional networks in the original code, and we will modify Encoder in the next commit.
  • As for the question 2, although the logvars and mus are produced from two separate Linear modules, the two Linear modules are both nn.Linear(hidden_dim, latent_dim), and there is no difference in the results of our model implementation.
  • As for the question 3, it is our negligence that the current model does not have alternate training. We will add RecVAETrainer in recbole.trainer.trainer.py in the next commit.

Thank you very much for your attention and kindly corrections, we will improve the RecVAE code this week.

@2017pxy 2017pxy merged commit b788a0a into RUCAIBox:0.2.x Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants