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 negative sampling RNGs. #39

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Fix negative sampling RNGs. #39

merged 1 commit into from
Apr 25, 2019

Conversation

sebpuetz
Copy link
Member

During refactoring, the ZipfRangeGenerator was unintentionally
replaced by an RNG. This commit undoes this change.

@sebpuetz sebpuetz requested a review from danieldk April 19, 2019 09:39
@sebpuetz
Copy link
Member Author

sebpuetz commented Apr 19, 2019

I'll wait with merging until the fix is verified through trained models.

During refactoring, the ZipfRangeGenerator was unintentionally
replaced by an RNG. This commit undoes this unintended change.
@sebpuetz
Copy link
Member Author

sebpuetz commented Apr 22, 2019

Well, there was yet another bug to be squashed. I squashed and pushed the fix already.
Structgram panicked because of out of bounds indices from negative samples. I can't remember why I put this code there:

            let negative = match self.skipgram_config.model {
                ModelType::StructuredSkipGram => {
                    let context_size = self.skipgram_config.context_size as usize;
                    let offset = output % (context_size * 2);
                    let rand_type = self.range_gen.next().unwrap();
                    // in structured skipgram the offset into the output matrix is calculated as:
                    // (vocab_idx * context_size * 2) + offset
                    rand_type * context_size * 2 + offset
                }
                ModelType::SkipGram => self.range_gen.next().unwrap(),
            };

But I guess that would've drawn a negative sample with the same offset as the original context.

@sebpuetz sebpuetz requested a review from danieldk April 22, 2019 21:58
@danieldk
Copy link
Member

Weird, that seems to originate from before BandedRangeGenerator.

@sebpuetz
Copy link
Member Author

Weird, that seems to originate from before BandedRangeGenerator.

I'm suspecting that I copy-pasted that from one of my working branches.

The skipgram model is done and is now only 0.2% below the pretrained model. 56.94% vs. 57.20%. Structgram will take a while, I didn't see that it crashed until last night.

@sebpuetz
Copy link
Member Author

Structgram is also done now, the newly trained model is 0.47% below the pretrained model at 59.53% vs. 60.10%. Do you think this difference can be due to random factors?
I won't merge this PR until you green light that.

@danieldk
Copy link
Member

danieldk commented Apr 25, 2019

Structgram is also done now, the newly trained model is 0.47% below the pretrained model at 59.53% vs. 60.10%. Do you think this difference can be due to random factors?

Hard to tell. I did see some variance between runs. I think 0.57% is within the realm of possibilities. Maybe you could train a basic topo model with both structgram models to verify that the results are the same for a downstream task?

(Of course, it may not give the best result as with zipf distribution parameter hacking, but it's a good extra sanity check.)

@sebpuetz
Copy link
Member Author

New embeddings on val 98.19 vs. old embeddings on val 98.08.

Which is actually a new over-all highscore for our topo experiments.

Anyways, I think this still confirms that we are getting comparable embeddings out of the new release. The .1 difference could be due to random effects in toponn or due to random effects in finalfrontier, I don't think it's possible to figure that out.

@danieldk
Copy link
Member

danieldk commented Apr 25, 2019 via email

@sebpuetz sebpuetz merged commit 16035a3 into master Apr 25, 2019
@sebpuetz sebpuetz deleted the sampling_fix branch April 25, 2019 15:27
@sebpuetz sebpuetz mentioned this pull request Oct 10, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants