Triplet sampling in NeighborLoader
#6004
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6004 +/- ##
==========================================
- Coverage 84.73% 84.66% -0.07%
==========================================
Files 361 361
Lines 20215 20185 -30
==========================================
- Hits 17129 17090 -39
- Misses 3086 3095 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Incorporated your review. Thank you! |
| assert int(batch['paper'].batch.max()) + 1 == 32 + 16 | ||
| # Check if each seed edge has a different source and dstination node: | ||
| assert batch['paper'].num_nodes >= 2 * (32 + 16) | ||
| assert int(batch['paper'].batch.max()) + 1 == 32 |
There was a problem hiding this comment.
Why isn't this 64? Won't the negative samples also have a disjoint batch?
There was a problem hiding this comment.
I personally think it makes more sense to group the negatives and positives to its own batch, in particular because they now share time information.
There was a problem hiding this comment.
Hmm, I see. But it seems weird that we now have more than 1 seed edge with the same batch number. Even though their computation graphs are in fact disjoint. But we can debate this later.
|
@wsad1 Updated based on your comments. |
| assert int(batch['paper'].batch.max()) + 1 == 32 + 16 | ||
| # Check if each seed edge has a different source and dstination node: | ||
| assert batch['paper'].num_nodes >= 2 * (32 + 16) | ||
| assert int(batch['paper'].batch.max()) + 1 == 32 |
There was a problem hiding this comment.
Hmm, I see. But it seems weird that we now have more than 1 seed edge with the same batch number. Even though their computation graphs are in fact disjoint. But we can debate this later.
This PR implements triplet sampling in link-level tasks, where we want only want to sample a negative destination node. This PR got quite large, especially because I stumbled upon some weird things in the current link-level pipeline on temporal graphs, e.g., we don't want to sample nodes that do not yet exist for a given edge label timestamp. When `neg_sampling="triplet"`, then the returned `Data` object holds: * a `src_index` which denotes the index from left-hand-side embeddings * a `dst_pos_index` which denotes the index from positive right-hand-side embeddings * a `dst_neg_index` which denotes the index from negative right-hand-side embeddings Overall, it introduces * `NegativeSamplingConfig` with `strategy=binary/triplet` * Deprecates `neg_sampling_ratio` in favor of `neg_sampling` * Introduces a new method called `neg_sampling` that respects the timestamp of nodes while sampling * Refactors and extends `sample_from_edges` => Overall, this function got quite large, and I will consider separating them into multiple functions in the future.
This PR implements triplet sampling in link-level tasks, where we want only want to sample a negative destination node.
This PR got quite large, especially because I stumbled upon some weird things in the current link-level pipeline on temporal graphs, e.g., we don't want to sample nodes that do not yet exist for a given edge label timestamp.
When
neg_sampling="triplet", then the returnedDataobject holds:src_indexwhich denotes the index from left-hand-side embeddingsdst_pos_indexwhich denotes the index from positive right-hand-side embeddingsdst_neg_indexwhich denotes the index from negative right-hand-side embeddingsOverall, it introduces
NegativeSamplingConfigwithstrategy=binary/tripletneg_sampling_ratioin favor ofneg_samplingneg_samplingthat respects the timestamp of nodes while samplingsample_from_edges=> Overall, this function got quite large, and I will consider separating them into multiple functions in the future.