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

GraphStore definition + Data and HeteroData integration #4816

Merged
merged 33 commits into from
Jun 21, 2022

Conversation

mananshah99
Copy link
Contributor

This PR defines a very lightweight MaterializedGraph that supports basic put and get operations of edge indices. It additionally lets Data and HeteroData inherit from and implement this abstraction.

@mananshah99 mananshah99 marked this pull request as ready for review June 17, 2022 19:08
@mananshah99 mananshah99 requested a review from rusty1s June 17, 2022 19:08
@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #4816 (f8b8d9e) into master (4b30b6d) will decrease coverage by 1.82%.
The diff coverage is 94.54%.

@@            Coverage Diff             @@
##           master    #4816      +/-   ##
==========================================
- Coverage   84.53%   82.70%   -1.83%     
==========================================
  Files         325      326       +1     
  Lines       17430    17518      +88     
==========================================
- Hits        14735    14489     -246     
- Misses       2695     3029     +334     
Impacted Files Coverage Δ
torch_geometric/data/feature_store.py 88.80% <90.00%> (ø)
torch_geometric/data/data.py 91.03% <93.02%> (+0.15%) ⬆️
torch_geometric/data/graph_store.py 95.00% <95.00%> (ø)
torch_geometric/data/hetero_data.py 93.84% <100.00%> (+0.20%) ⬆️
torch_geometric/typing.py 100.00% <100.00%> (ø)
torch_geometric/nn/models/dimenet_utils.py 0.00% <0.00%> (-75.52%) ⬇️
torch_geometric/nn/models/dimenet.py 14.51% <0.00%> (-53.00%) ⬇️
torch_geometric/nn/conv/utils/typing.py 81.25% <0.00%> (-17.50%) ⬇️
torch_geometric/nn/inits.py 67.85% <0.00%> (-7.15%) ⬇️
torch_geometric/nn/resolver.py 90.00% <0.00%> (-6.67%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b30b6d...f8b8d9e. Read the comment docs.

"An edge layout is required to store an edge index, but one "
"was not provided.")

# NOTE implementations should take care to ensure that `SparseTensor`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that mean in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this to work with Tuple[Tensor, Tensor] directly; lmkwyt!



@dataclass
class EdgeAttr(CastMixin):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This term is now a bit overloaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what manner? I think it aligns well with TensorAttr in FeatureStore (they are both attributes of objects we are trying to add to the respective stores).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually refer to data.edge_attr for edge attributes/features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I think it's probably okay to leave this overloaded term for now (mostly since I can't think of a better one), and if you have any ideas we can do a quick refactor.

Base automatically changed from feature_store_pt1 to master June 20, 2022 17:20
@mananshah99
Copy link
Contributor Author

@rusty1s @Padarn @wsad1: thank you for the feedback! This PR has been updated accordingly. A few notable changes:

  • MaterializedGraph is now named GraphStore
  • EdgeAttr has been cleaned up, and overridden in Data (just like TensorAttr)
  • We now support 'coo', 'csr', and 'csc' as arguments (instead of the full Enum definition)
  • GraphStore now directly operates on EdgeTensorType = Tuple[Tensor, Tensor], as suggested by @rusty1s. This alleviates issues with integrating SparseTensor and cleans up the implementation.
    • Additionally, Data and HeteroData's implementations of GraphStore abstract methods are unified and quite simple. For put, based on the passed EdgeLayout, they construct a corresponding Tensor or SparseTensor and store that object internally. For get, based on the passed EdgeLayout, they construct a Tuple[Tensor, Tensor] and return this back to the caller.

Let me know what you think.

@mananshah99 mananshah99 changed the title MaterializedGraph definition + Data and HeteroData integration GraphStore definition + Data and HeteroData integration Jun 21, 2022
Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am super impressed. This is clean.



@dataclass
class EdgeAttr(CastMixin):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually refer to data.edge_attr for edge attributes/features.

@mananshah99 mananshah99 merged commit b274fbd into master Jun 21, 2022
@mananshah99 mananshah99 deleted the feature_store_pt2 branch June 21, 2022 23:12
@Padarn
Copy link
Contributor

Padarn commented Jul 3, 2022

Sorry I was really busy and didn't read your updates earlier @mananshah99 - looks awesome and thanks for your responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants