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

Bpr fix out of bound error #13

Conversation

felixmaximilian
Copy link

Potential fix for ibayer/fastFM#61 (#61).
The problem with this while loop is, the following:
While p_n is the reason why the conditional OR evaluates to true (even the p_p part is false), i_to_update can decide to update p_p which leads to an infinite loop and the p_p accesses Ax[p_p] on an undefined memory slot.

Within the BPR optimization loop, the pointer into the data can run out
of bounds, because the condition in while loop only checks if one
pointer of the examples (negative or positive) is still within
reasonable bounds, but then the other pointer might be increased in the
end. THis fix double checks that the pointer stays within bounds.
@ibayer
Copy link
Owner

ibayer commented Jul 14, 2016

@felixmaximilian
Thanks for the PR!

I can crash the fitting process by passing just one specific learning pair, but the whole feature matrix. This is the exact learning pair you see in the stack above.

Can you post your one pair breaking example here? I didn't get it from the debug output.

I probability have time tomorrow to have a closer look in order to check if there are related issues.

@felixmaximilian
Copy link
Author

felixmaximilian commented Jul 15, 2016

You can see the pair in the gdb info locals output in the issue report. It's called sample_row_p and n. But without the data it won't help you understand I think. Unfortunately I cannot post the feature matrix.

@ibayer
Copy link
Owner

ibayer commented Jul 18, 2016

@felixmaximilian
The issue you found shows up at more places (see my referenced PR).

@ibayer
Copy link
Owner

ibayer commented Jul 18, 2016

@felixmaximilian
Can we close this now?

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