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.

Sign up for GitHub

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

Jump to bottom

Include support for GeoIMC algorithm #1142

Merged
merged 4 commits into from
Jul 28, 2020
Merged

Include support for GeoIMC algorithm #1142

merged 4 commits into from
Jul 28, 2020

Conversation

SatyadevNtv
Copy link
Contributor

Description

Add GeoIMC algorithm. Checkout 00_quick_start/geoimc_movielens.ipynb for a quick overview.

Related Issues

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging and not master.

@SatyadevNtv
Include support for GeoIMC algorithm
472bf95 
Checkout 00_quick_start/geoimc_movielens.ipynb
for a quick overview
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

@ghost
Copy link

ghost commented Jul 11, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

miguelgfierro
miguelgfierro reviewed Jul 14, 2020
View reviewed changes
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

this is super interesting, thanks for adding it, I did a first pass, please take a look at the comments. I'll be OOW for some days so please expect a delayed response.
Thanks!

reco_utils/recommender/geoimc/GeoIMCalgorithm.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCdata.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCpredict.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCutils.py Outdated Show resolved Hide resolved
examples/00_quick_start/geoimc_movielens.ipynb Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCutils.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCutils.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCdata.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCdata.py Outdated Show resolved Hide resolved
reco_utils/recommender/geoimc/GeoIMCdata.py Outdated Show resolved Hide resolved
@miguelgfierro
Copy link
Collaborator

hey another comment, I was looking the other day at the algo Pratik and Bamdev did RLRMC, which I really liked. It doesn't have a ranking method (like recommend_k_items). Out of curiosity, for GeoIMC or even for RLRMC, is it possible to have a ranking method? is mathematically sound to do it?

FYI @pratikjawanpuria @bamdevm

@pratikjawanpuria
Copy link
Collaborator

hey another comment, I was looking the other day at the algo Pratik and Bamdev did RLRMC, which I really liked. It doesn't have a ranking method (like recommend_k_items). Out of curiosity, for GeoIMC or even for RLRMC, is it possible to have a ranking method? is mathematically sound to do it?

FYI @pratikjawanpuria @bamdevm

Yes Miguel, it should be possible to have such a ranking method. Thanks for the input. We will include it in GeoIMC and also add it in RLRMC.

@pratikjawanpuria
Copy link
Collaborator

(sorry I pressed "close and comment" by mistake)

@SatyadevNtv
Update to be compatible with upstream's repo.
b19a52b 
- Add License information.
- Rename `GeoIMC` -> `geoimc_` in filenames.
- Remove features blob from repo and download from upstream's
  data storage URL.
- Add missing docstrings.
- Move from `print` -> `logger` based logging.
@miguelgfierro
Copy link
Collaborator

hey @SatyadevNtv, this is really good. Just one final ask, people normally add tests to make sure that everything works. Here you can read more about it: https://github.com/microsoft/recommenders/tree/master/tests

Would it be possible to add unit tests for the code and an integration test for the notebook?

@SatyadevNtv
Copy link
Contributor Author

hey @SatyadevNtv, this is really good. Just one final ask, people normally add tests to make sure that everything works. Here you can read more about it: https://github.com/microsoft/recommenders/tree/master/tests

Would it be possible to add unit tests for the code and an integration test for the notebook?

Yes. It should be.
I will include them.

Thanks.

@SatyadevNtv
geoimc: Add unit, integration tests
6a3a5aa 
- Move `binarize` to `common/python_utils.py`
- Remove hardcoded normalized value
@SatyadevNtv
Copy link
Contributor Author

hey @SatyadevNtv, this is really good. Just one final ask, people normally add tests to make sure that everything works. Here you can read more about it: https://github.com/microsoft/recommenders/tree/master/tests

Would it be possible to add unit tests for the code and an integration test for the notebook?

Included them at 6a3a5aa

miguelgfierro
miguelgfierro approved these changes Jul 28, 2020
View reviewed changes
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

this is really good, thanks

@miguelgfierro miguelgfierro merged commit 221feb5 into recommenders-team:staging Jul 28, 2020
@miguelgfierro
Copy link
Collaborator

hey @SatyadevNtv, thanks for the contribution, feel free to add your name in authors if you wish https://github.com/Microsoft/Recommenders/wiki/How-to-add-your-name-as-a-contributor-to-the-repo#add-your-name-in-authorsmd

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

@miguelgfierro miguelgfierro miguelgfierro approved these changes

@anargyri anargyri Awaiting requested review from anargyri anargyri is a code owner

@gramhagen gramhagen Awaiting requested review from gramhagen gramhagen is a code owner

@loomlike loomlike Awaiting requested review from loomlike loomlike is a code owner

@yueguoguo yueguoguo Awaiting requested review from yueguoguo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SatyadevNtv @miguelgfierro @pratikjawanpuria