Public API Refactor - #382
Conversation
ntgbaoo
left a comment
ntgbaoo
left a comment
There was a problem hiding this comment.
There are a few parts of some docs that need to be updated. Other than that, looks good to me.
quickstart.md need to be updated as well.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| for batch in tqdm(loader): | ||
| opt.zero_grad() | ||
| y_true = batch.dynamic_node_feats | ||
| y_true = batch.node_x |
There was a problem hiding this comment.
in the label PR, this becomes node_y
| num_nodes = torch.max(self.edge_index).item() + 1 # 0-indexed | ||
| if self.node_ids is not None: | ||
| num_nodes = max(num_nodes, torch.max(self.node_ids).item() + 1) # 0-indexed | ||
| if self.node_x_nids is not None: |
There was a problem hiding this comment.
interestingly for node_y_nids in next PR, what if max_node exist in node_y what would that mean?
to me it would just be invalid? you are predicting for an unknown node, it should be invalid
There was a problem hiding this comment.
Not sure about that. Seems like something that could easily happen?
There was a problem hiding this comment.
if the node doesn't exist in the graph then you can't predict for it? to discuss in next PR
ntgbaoo
left a comment
ntgbaoo
left a comment
There was a problem hiding this comment.
Thanks @Jacob-Chmura for a very comprehensive PR. Took me a while to go over it lol.
| return edge_type | ||
|
|
||
| @_logged_cached_property | ||
| def _node_events_cpu(self) -> Tuple[Tensor, Tensor]: |
There was a problem hiding this comment.
| def _node_events_cpu(self) -> Tuple[Tensor, Tensor]: | |
| def _node_x_cpu(self) -> Tuple[Tensor, Tensor]: |
There was a problem hiding this comment.
I think this conflicts too much with the backend call get_node_x which is getting dynamic node features
|
|
||
| @_logged_cached_property | ||
| def _node_events_cpu(self) -> Tuple[Tensor, Tensor]: | ||
| return self._storage.get_node_events(self._slice) |
There was a problem hiding this comment.
Do we want to change implementation of storage to get_node_x? Eventually, I think we should since there will be get_node_y
There was a problem hiding this comment.
hmm, I'll consider in the subsequent label PR. My only concern is that we already have get_node_x as the backend method to get dynamic node features.
Arguably we can consolidate these all somehow.
| @@ -70,12 +70,12 @@ def get_edges(self, slice: DGSliceTracker) -> Tuple[Tensor, Tensor, Tensor]: | |||
| def get_node_events(self, slice: DGSliceTracker) -> Tuple[Tensor, Tensor]: | |||
There was a problem hiding this comment.
| def get_node_events(self, slice: DGSliceTracker) -> Tuple[Tensor, Tensor]: | |
| def get_node_x(self, slice: DGSliceTracker) -> Tuple[Tensor, Tensor]: |
3 participants
Close #377