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

Add ComparER models #392

Merged
merged 2 commits into from
Mar 9, 2021
Merged

Add ComparER models #392

merged 2 commits into from
Mar 9, 2021

Conversation

lthoang
Copy link
Member

@lthoang lthoang commented Mar 8, 2021

Description

Related Issues

Checklist:

  • I have added tests.
  • I have updated the documentation accordingly.
  • I have updated README.md (if you are adding a new model).
  • I have updated examples/README.md (if you are adding a new example).
  • I have updated datasets/README.md (if you are adding a new dataset).

@lthoang lthoang requested a review from tqtg March 8, 2021 19:32
@tqtg
Copy link
Member

tqtg commented Mar 9, 2021

@lthoang isn't better to have two variants merged into one model CompareER and having an example to demonstrate the differences between the two?

@lthoang
Copy link
Member Author

lthoang commented Mar 9, 2021

@tqtg Two variants build upon two different base models, they use different set of parameters. It should be clearer and easier to maintain.

@tqtg
Copy link
Member

tqtg commented Mar 9, 2021

@lthoang yes, but they can be organized under one module. We don't have to separate them into two completely different modules.

@lthoang
Copy link
Member Author

lthoang commented Mar 9, 2021

@tqtg I put both variants into comparer module. For the usage examples, I put the reference link to ComparER repository

@tqtg
Copy link
Member

tqtg commented Mar 9, 2021

@lthoang you've changed the Amazon Toy dataset in this PR as well. Could you check if any other examples also using the dataset and backward compatible?

@lthoang
Copy link
Member Author

lthoang commented Mar 9, 2021

@tqtg checked.

@tqtg tqtg merged commit bd9ec51 into PreferredAI:master Mar 9, 2021
tqtg pushed a commit to amirj/cornac that referenced this pull request May 22, 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.

2 participants