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

tfidf runtime enhancement changes #1571

Merged
merged 3 commits into from
Dec 3, 2021
Merged

tfidf runtime enhancement changes #1571

merged 3 commits into from
Dec 3, 2021

Conversation

AdityaSoni19031997
Copy link
Contributor

Description

The minimal changes made to the tf_idf_utils file in this PR helps in reducing the overall runtime by avoiding the looping and slicing the pandas DataFrame.

The PR helps in resolving the issue raised in #1568.

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 branch and not to main branch.

Looking forward to the feedback on the PR and any other changes needed.

Best,
Aditya.

@ghost
Copy link

ghost commented Nov 30, 2021

CLA assistant check
All CLA requirements met.

@AdityaSoni19031997
Copy link
Contributor Author

AdityaSoni19031997 commented Dec 2, 2021

cc @miguelgfierro
Can the workflow be approved so that we can see the results now?

Thanks!

@miguelgfierro
Copy link
Collaborator

@AdityaSoni19031997 approved!

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.

Really good contribution, thanks!

@miguelgfierro miguelgfierro merged commit a277138 into recommenders-team:staging Dec 3, 2021
@AdityaSoni19031997
Copy link
Contributor Author

Thanks Miguel! Looking forward to explore bits and pieces of this repository!

@AdityaSoni19031997
Copy link
Contributor Author

AdityaSoni19031997 commented Dec 3, 2021

cc @miguelgfierro I am just curious, Why the repository doesn't use PyTorch?

@miguelgfierro
Copy link
Collaborator

Why the repository doesn't use PyTorch?

hehe, good question. Parts of the code in this repo are old, Microsoft Research used to use TF, and now they are moving to PyTorch. In future releases you will probably see more code in PyTorch, but we will keep supporting TF.

@anargyri has been having recently some fun with TF #1565

@AdityaSoni19031997
Copy link
Contributor Author

Haha, Not a TF fan! I like PyTorch more than TF.

Is there any brach where people are working in porting the snips to torch?

Thanks.

@miguelgfierro
Copy link
Collaborator

Is there any brach where people are working in porting the snips to torch?

we are not planning to port the current code to PyTorch, the future one will be developed in PyTorch

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