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

HeteroData.is_undirected() #4604

Merged
merged 3 commits into from
May 10, 2022
Merged

HeteroData.is_undirected() #4604

merged 3 commits into from
May 10, 2022

Conversation

rusty1s
Copy link
Member

@rusty1s rusty1s commented May 9, 2022

No description provided.

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #4604 (e34551b) into master (cd6c1f7) will increase coverage by 0.01%.
The diff coverage is 97.91%.

@@            Coverage Diff             @@
##           master    #4604      +/-   ##
==========================================
+ Coverage   82.80%   82.81%   +0.01%     
==========================================
  Files         315      315              
  Lines       16599    16605       +6     
==========================================
+ Hits        13744    13752       +8     
+ Misses       2855     2853       -2     
Impacted Files Coverage Δ
torch_geometric/data/hetero_data.py 93.84% <97.87%> (+0.87%) ⬆️
torch_geometric/data/storage.py 80.06% <100.00%> (ø)

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 cd6c1f7...e34551b. Read the comment docs.

if len(edge_indices) == 1: # Memory-efficient `torch.cat`:
edge_index = edge_indices[0]
elif len(edge_indices) > 0:
edge_index = torch.cat(edge_indices, dim=-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

[naive question] would it be better to cat incrementally in the loop which determines the edge_slices?

@Padarn
Copy link
Contributor

Padarn commented May 10, 2022

Out of scope - but do you think we should somehow cache the result of 'is undirected'? I'm wondering if anyone uses it for example in a forward pass of a model, in which case it'll be a very expensive operation to repeat many times.

Maybe that's not something anyone would do though.

@rusty1s
Copy link
Member Author

rusty1s commented May 10, 2022

I think that is an interesting idea. One thing that complicates this is that PyG would need to track changes to Data and invalidate the cache, or define some method to freeze the current data object and disallow modifications. Let me know what you think!

@rusty1s rusty1s merged commit 8fdf895 into master May 10, 2022
@rusty1s rusty1s deleted the hetero_is_undirected branch May 10, 2022 11:01
@Padarn
Copy link
Contributor

Padarn commented May 10, 2022

Yeah that is true, invalidating the cache does seem fairly error-prone. It doe seem possible given the Data objects do all of their setting of attributes via the _store. Perhaps it would be fairly safe to just always clear the cache when an attribute is changed.

I like the idea of something like FrozenData, but outside of this I don't know what other use cases it might serve.

@rusty1s
Copy link
Member Author

rusty1s commented May 11, 2022

Agree. I think this is an interesting idea that we may want to integrate once we find more use-cases for it :)

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.

ToUndirect and is_undirect does not match on heterogeneous graphs
2 participants