search

stanfordnlp / GloVe

Software in C and data files for the popular GloVe model for distributed word representations, a.k.a. word vectors or embeddings
Apache License 2.0
6.86k stars 1.51k forks source link

Detect overflow earlier. #68

Closed kosloot closed 4 years ago

kosloot commented 7 years ago

On line 339 overflow is detected. But in the 'symmetric' case, the 'ind' variable gets incremented twice in the loop starting at line 356 and it can therefore still overflow. Using the condition: if (ind >= overflow_length - 2*window_size)

this is prevented.

kosloot commented 7 years ago

Added another fix too: in glove.c the W and gradsq arrays weren't completely initialized. The initialization was done in an expensive way too, with 4 (incorrect) for loops.

In my proposal there is 1 loop left, and ALL items are initialized.

ghost commented 7 years ago

Okay, we just added travis, so this looks like it's good to merge. I know this is a little strange to ask given that the current indentation is so bad, but would you mind resubmitting with your initialize_parameters() indentation set to 4 spaces, like the rest of the code?

kosloot commented 7 years ago

Indentation is a mess indeed. (8 spaces too....) A new pull request #73 is created now.

AngledLuffa commented 4 years ago

I made the changes I suggested above and merged it, giving you credit. If you can explain how the loop bounds are wrong, I'll fix it - otherwise it seems to work for now