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

Fix EFM._build_matrices() method #616

Closed
wants to merge 1 commit into from

Conversation

hieuddo
Copy link
Member

@hieuddo hieuddo commented May 4, 2024

Description

The two methods knows_user and knows_item check whether uid (or iid) is smaller than self.num_users (or self.num_items), which doesn't exist because fit() was never executed.

This commit tries to fix an error in recommender-systems tutorial notebook 7, where EFM.train_set is assigned and _build_matrices() is executed before fit():

efm = EFM()
efm.train_set = rs.train_set
_, X, Y = efm._build_matrices(rs.train_set)

@hieuddo hieuddo added the bug Something isn't working label May 4, 2024
@hieuddo hieuddo requested review from lthoang and tqtg May 4, 2024 02:08
@tqtg
Copy link
Member

tqtg commented May 4, 2024

@hieuddo @lthoang how about modifying the tutorial to the following?

efm = EFM(max_iter=0)
efm.fit(train_set=rs.train_set)

I think it serves the same purpose and looks cleaner.

@hieuddo
Copy link
Member Author

hieuddo commented May 5, 2024

@tqtg I think your suggestion makes sense. efm.fit(train_set=rs.train_set) seems more conventional than directly assign train_set by efm.train_set = rs.train_set.

Anyway, could you help to release new cornac version with the recent Recommender.rank() fix so that EFM can run smoothly?

@tqtg
Copy link
Member

tqtg commented May 6, 2024

@hieuddo we have a new version released. Should we check if the above approach works for the tutorial? and do we need to work on this change anymore?

@hieuddo
Copy link
Member Author

hieuddo commented May 7, 2024

@tqtg Yes, it works. There is no need for this change. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants