LinkNeighborLoader: Tuple[FeatureStore, GraphStore] support
#5037
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5037 +/- ##
==========================================
- Coverage 82.89% 82.87% -0.02%
==========================================
Files 331 331
Lines 18160 18197 +37
==========================================
+ Hits 15053 15081 +28
- Misses 3107 3116 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. |
There was a problem hiding this comment.
Approving to unblock: would be great to get rid of the num_src_nodes and num_dst_nodes arguments in the constructor. These should always be inferred IMO (even for GraphStore). The logic of inferring input edges is a bit complicated as well. I think we should either simplify it or refactor it to allow for GraphStore.coo(edge_type).
| edge_types.index(self.input_type)].size | ||
|
|
||
| # Otherwise: | ||
| elif num_src_nodes is None or num_dst_nodes is None: |
There was a problem hiding this comment.
Can‘t we access via FeatureStore.size in this case? Might be cooler since it will not require a change in input arguments.
There was a problem hiding this comment.
FeatureStore.size is kind of broken since we currently need to provide an attribute to get the size, which is not available in general, which is why this approach was chosen :( Happy to refactor once we clean up size.
Supports the feature store / graph store abstraction for the link-level neighbor loader.