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

Changed the interface of coalesce to only return single edge_index information in case the edge_attr argument is not specified #6879

Merged
merged 6 commits into from
Mar 8, 2023

Conversation

rusty1s
Copy link
Member

@rusty1s rusty1s commented Mar 8, 2023

No description provided.

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #6879 (5d19333) into master (581dd01) will increase coverage by 0.00%.
The diff coverage is 95.23%.

❗ Current head 5d19333 differs from pull request most recent head b857e10. Consider uploading reports for the commit b857e10 to get more accurate results

@@           Coverage Diff           @@
##           master    #6879   +/-   ##
=======================================
  Coverage   91.53%   91.53%           
=======================================
  Files         427      427           
  Lines       23173    23166    -7     
=======================================
- Hits        21211    21205    -6     
+ Misses       1962     1961    -1     
Impacted Files Coverage Δ
torch_geometric/utils/undirected.py 90.24% <66.66%> (-0.67%) ⬇️
torch_geometric/io/tu.py 90.58% <100.00%> (-0.22%) ⬇️
torch_geometric/transforms/line_graph.py 100.00% <100.00%> (ø)
torch_geometric/transforms/two_hop.py 100.00% <100.00%> (ø)
torch_geometric/utils/coalesce.py 100.00% <100.00%> (+2.32%) ⬆️
torch_geometric/utils/grid.py 100.00% <100.00%> (ø)
torch_geometric/utils/sort_edge_index.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rusty1s rusty1s self-assigned this Mar 8, 2023
@rusty1s rusty1s merged commit 8e6b325 into master Mar 8, 2023
@rusty1s rusty1s deleted the coalesce branch March 8, 2023 12:44
@EdisonLeeeee
Copy link
Contributor

@rusty1s thank you for the update. I've been struggling with this discrepancy between different functions, such as coalesce and add_self_loops.

However, this is indeed a breaking change in PyG. I'm concerned that there may be compatibility issues for users who upgrade from older versions of PyG. Most importantly, the returned edge_index is unpackable, which means that the same code may cause potential errors when edge_attr is None. For example:

# for PyG<=2.2
edge_index, _ = coalesce(...) # correct

# for PyG >=2.3
edge_index, _ = coalesce(...) # wrong but work properly, equivalent to row, col = edge_index

The worst-case scenario is that code written with an older version will still work in the latest version, but return incorrect results. Should we take steps to avoid this situation?

@rusty1s
Copy link
Member Author

rusty1s commented Mar 8, 2023

Yes, this is a breaking change and will be highlighted in the release notes. I am not totally convinced it will impact a lot of users, as sort_edge_index and coalesce are more used internally, and it would only cause problems if you would call these functions via edge_index = coalesce(edge_index, None) (which seems rare). If you have any idea on how to make this backward compatible, I would be happy to adapt it.

@EdisonLeeeee
Copy link
Contributor

Thank you for your clarification. You are right. But I think we can add a warning in the doc-string, WDYT?

@rusty1s
Copy link
Member Author

rusty1s commented Mar 10, 2023

Yes, sg. Will take care of it.

@rusty1s
Copy link
Member Author

rusty1s commented Mar 10, 2023

Done in #6893.

@EdisonLeeeee
Copy link
Contributor

Thank you!

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.

None yet

2 participants