NeighborLoader: support temporal sampling with (FeatureStore, GraphStore)
#4929
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4929 +/- ##
==========================================
- Coverage 82.71% 82.70% -0.01%
==========================================
Files 330 330
Lines 17870 17882 +12
==========================================
+ Hits 14781 14790 +9
- Misses 3089 3092 +3
Continue to review full report at Codecov.
|
| if isinstance(input_nodes, str): | ||
| num_nodes = feature_store.get_tensor_size(input_nodes)[0] | ||
| return input_nodes, range(num_nodes) | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
Why do we raise here instead of fix it? Shouldn't feature_store.get_tensor_size(TensorAttr(group_name=input_nodes))[0] fix this?
There was a problem hiding this comment.
That get tensor size call wouldn't work as intended since get tensor rise needs both a group name and are name, hence the raise
There was a problem hiding this comment.
So what we could do is to select the first TensorAttr that has group_name==input_nodes, right? But yeah, not ideal.
There was a problem hiding this comment.
Yeah, not a huge fan of that solution; will leave for a later PR if we have to go that route.
|
|
||
| if isinstance(input_nodes, (list, tuple)): | ||
| assert len(input_nodes) == 2 | ||
| assert isinstance(input_nodes[0], str) | ||
|
|
||
| node_type, input_nodes = input_nodes | ||
| if input_nodes is None: | ||
| num_nodes = feature_store.get_tensor_size(input_nodes)[0] | ||
| return input_nodes[0], range(num_nodes) | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
feature_store.get_tensor_size(TensorAttr(group_name=None))[0]There was a problem hiding this comment.
Same reason as above.
for more information, see https://pre-commit.ci
Supports temporal sampling by performing the equivalent of a
collect()operation onFeatureStoreto constructnode_time_dict. Also fixes a minor bug inHeteroData.put_tensor.