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

added .to(self.device) - bug fixed for SLIMElastic #1745

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

giuspillo
Copy link

@giuspillo giuspillo commented Apr 21, 2023

With this changes, I solved a bug related to the model SLIMElastic; I found issues related to the instruction with torch.cat() in this file, the tensor seemed to be partially on the GPU and partially on the CPU. This solved.

Moreover, this source code includes the fix for the bug reported in this issue.

if self.register.need("data.count_users"):
self.data_struct.set("data.count_users", train_data.dataset.user_counter)
self.data_struct.set("data.count_items", train_data._dataset.user_counter)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe line 94 is self.data_struct.set("data.count_users", train_data._dataset.user_counter), since need("data.count_users").

Copy link
Author

Choose a reason for hiding this comment

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

It is probably correct, but in the version I downloaded by using pip install recbole==1.1.1 dgl
line 94 is
self.data_struct.set("data.count_items", train_data.dataset.user_counter)

I also checked the source code on my personal laptop and I can confirm that.

Copy link
Member

Choose a reason for hiding this comment

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

@giuspillo Yes, it is indeed an existing Typo, but we also hope that you can change it together so that we don't have to open a new PR to fix this issue. Thanks very much for your support and enhancement to RecBole!

Copy link
Author

Choose a reason for hiding this comment

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

Line 94 fixed and committed :)

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.

3 participants