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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰馃悢 Fixing bug with index-error for predict.consume_scores #1157

Merged
merged 8 commits into from
Nov 19, 2022

Conversation

AImenes
Copy link
Contributor

@AImenes AImenes commented Nov 6, 2022

Link to the relevant Bug(s)

fix #1156

Description of the Change

Remainder is actually quotient, and quotient is actually remainder. The Change flips these.

Fixed IndexError where it had flipped the the remainder and quotient. (Error not visible on original code snippet as it had the same amount of entities and relation (making it a square). If say we had 1000 entities and 1 relation (1000 x 1) it would originally iterate (1 x 1000) getting an index error. This flips these indices.

My guess is this error occurred when the working example had same amount of entities and relations, so it would work regardless just iterating horizontally instead of vertically.

Possible Drawbacks

  • If any other methods utilizes __getitem__ (line 876-878), it will also flip for them.

Verification Process

  • Tried AllPredictionDataset as well, and it appears unaffected.

Release Notes

  • Fixed iteration index-error for prediction bug

AImenes and others added 4 commits November 6, 2022 10:04
Fixed indexError where it had flipped the the remainder and quotient. (Error not visible on original code snippet as it had the same amount of entities and relation (making it a square). If say we had 1000 entities and 1 relation (1000 x 1) it would orginially iterate (1 x 1000) getting an index error. This flips these indices.
@mberr
Copy link
Member

mberr commented Nov 6, 2022

Hi @AImenes ,

thanks for your pr! I added a small test case swhich would have discovered the previous error.

Copy link
Member

@cthoyt cthoyt left a comment

Choose a reason for hiding this comment

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

Thank you @AImenes !

@mberr mberr enabled auto-merge (squash) November 19, 2022 13:52
@mberr
Copy link
Member

mberr commented Nov 19, 2022

pipeline failures -> #1170

@mberr mberr changed the title Fixing bug with index-error for predict.consume_scores 馃悰馃悢 Fixing bug with index-error for predict.consume_scores Nov 19, 2022
@mberr mberr disabled auto-merge November 19, 2022 17:01
@mberr mberr enabled auto-merge (squash) November 19, 2022 17:01
@cthoyt cthoyt disabled auto-merge November 19, 2022 17:44
@cthoyt cthoyt enabled auto-merge (squash) November 19, 2022 17:44
@cthoyt cthoyt merged commit ba3b153 into pykeen:master Nov 19, 2022
@cthoyt
Copy link
Member

cthoyt commented Nov 19, 2022

Thanks for being patient @AImenes! We made a few minor updates and merged it.

@AImenes
Copy link
Contributor Author

AImenes commented Dec 13, 2022

Thank you! 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants