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

Correct MIND user behavior history construction #2054

Conversation

thaiminhpv
Copy link
Contributor

@thaiminhpv thaiminhpv commented Jan 8, 2024

Description

This PR fixes #1258 - the construction of user behavior history when loading MIND data.

Changed from

history = [0] * (self.his_size - len(history)) + history[ : self.his_size ]

to

history = [0] * (self.his_size - len(history)) + history[ -self.his_size : ]

Related Issues

#1258

References

#1258 (comment)

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.

cc @yjw1029

@thaiminhpv thaiminhpv changed the title Thaiminhpv/correct mind user behavior history construction Correct MIND user behavior history construction Jan 8, 2024
@@ -119,7 +119,7 @@ def init_behaviors(self, behaviors_file):

history = [self.nid2index[i] for i in history.split()]
history = [0] * (self.his_size - len(history)) + history[
: self.his_size
-self.his_size :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @thaiminhpv, thanks for the contribution. It seems some files got mixed up in your PR.

To fix it, please change the PR to staging, and sign the commit.

Let me know if you have any difficulty or need any help

Copy link
Contributor Author

@thaiminhpv thaiminhpv Jan 8, 2024

Choose a reason for hiding this comment

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

@miguelgfierro Thank you, I've changed merge target branch to the staging branch. My commit was also signed. The diff contains only 1 commit now. Please have a look at it.

@thaiminhpv thaiminhpv changed the base branch from main to staging January 8, 2024 06:25
@SimonYansenZhao
Copy link
Collaborator

SimonYansenZhao commented Jan 8, 2024

@miguelgfierro The authentication issue from non-maintainers is still there. See the failing tests in this PR.

Screenshot 2024-01-09 at 00 10 11

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.

@SimonYansenZhao you are right. We need to fix this #1853.

I think we can merge to staging, if the nightly fails because of this change, we can review.

@miguelgfierro miguelgfierro merged commit e3e3ee7 into recommenders-team:staging Jan 8, 2024
1 of 20 checks passed
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.

[ASK] Questions about the construction of user history
3 participants