-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Zhangya #1415
Zhangya #1415
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great progress @YanZhangADS!
@YanZhangADS just a small nitpick, sometimes the branch names get confusing so it's better if you can put more descriptive naming in the branch |
@YanZhangADS I think another thing it would be good to update is the readme https://github.com/microsoft/recommenders/tree/main/docs if you have already reviewed the html files, let me know, and I'll do another pass |
Should I check-in all the html files in "build" folder to my branch? How would you suggest to update the doc? It looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done one pass to review the code and there are components missing. Here a PR with some of the fixes: #1425
Inside the folder recommenders.deeprec.models
we need to add the modules of graphrec and sequentials. Also, since DeepRec is a lib inside our lib, it would be cool to have a second level in the doc, so in the index of modules, when you press DeepRec, there is a menu with all the algos.
@zhangya let me know when you want me to take a look again
* review 📝 in common * wip * deprec * fix 📝 Co-authored-by: miguelgfierro <[email protected]>
* 🐛 in 📝 iterators * 🐛 in 📝 iterators * 🐛 in 📝 iterators * 🐛 in 📝 iterators * 🐛 in 📝 iterators * 🐛 in 📝 iterators 🐛 * 🐛 in 📝 iterators 🐛 Co-authored-by: miguelgfierro <[email protected]>
* common * common * movielens * cord * mind * 🐛 * downlowd * deeprec * deeprec * 💥 * Update dataset.rst * Update python_utils.py * typo * Update python_utils.py * Update pandas_df_utils.py * Update sparse.py * Update sparse.py Follow PEP8 for variable name Co-authored-by: miguelgfierro <[email protected]> Co-authored-by: Andreas Argyriou <[email protected]> Co-authored-by: YanZhangADS <[email protected]>
There was a problem hiding this 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 @YanZhangADS, congrats! really good job
Description
match recommenders/docs/source/*.rst files with code
Related Issues
Checklist:
staging branch
and not tomain branch
.