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

Refactor TopkPooling into SelectTopK #7308

Merged
merged 17 commits into from
May 8, 2023
Merged

Refactor TopkPooling into SelectTopK #7308

merged 17 commits into from
May 8, 2023

Conversation

wsad1
Copy link
Member

@wsad1 wsad1 commented May 6, 2023

Towards #6455.
Do not review before #7307 is merged.

@wsad1 wsad1 requested a review from EdisonLeeeee as a code owner May 6, 2023 06:00
@wsad1 wsad1 requested a review from rusty1s May 6, 2023 06:00
@github-actions github-actions bot added the nn label May 6, 2023
@wsad1 wsad1 changed the base branch from master to pool_update May 6, 2023 06:00
@wsad1 wsad1 self-assigned this May 6, 2023
@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #7308 (114b9a4) into master (c566f5c) will increase coverage by 0.11%.
The diff coverage is 97.43%.

❗ Current head 114b9a4 differs from pull request most recent head 1cbb5ef. Consider uploading reports for the commit 1cbb5ef to get more accurate results

@@            Coverage Diff             @@
##           master    #7308      +/-   ##
==========================================
+ Coverage   91.40%   91.52%   +0.11%     
==========================================
  Files         437      438       +1     
  Lines       24306    24337      +31     
==========================================
+ Hits        22218    22274      +56     
+ Misses       2088     2063      -25     
Impacted Files Coverage Δ
torch_geometric/nn/pool/select/topk.py 96.82% <96.82%> (ø)
torch_geometric/nn/pool/asap.py 90.41% <100.00%> (ø)
torch_geometric/nn/pool/pan_pool.py 95.74% <100.00%> (+0.09%) ⬆️
torch_geometric/nn/pool/sag_pool.py 100.00% <100.00%> (ø)
torch_geometric/nn/pool/select/__init__.py 100.00% <100.00%> (+100.00%) ⬆️
torch_geometric/nn/pool/select/base.py 96.42% <100.00%> (+96.42%) ⬆️
torch_geometric/nn/pool/topk_pool.py 100.00% <100.00%> (ø)

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

@wsad1 wsad1 mentioned this pull request May 6, 2023
4 tasks
Base automatically changed from pool_update to master May 8, 2023 08:07
@rusty1s
Copy link
Member

rusty1s commented May 8, 2023

Hi @wsad1, I modified this PR quite a bit. Hope the changes work for you:

  • I moved attention score computation to SelectTopK - I think this belongs there.
  • Added tests for SelectTopK
  • I reverted the addition of TopKConnect - Can we add that separately with corresponding test cases? I would also be in favor of renaming that to something more general, e.g., FilterGraph? The name should make it clear that we only maintain the nodes present in select_output.node_index. It may also require changes since it should respect the assignment matrix coming from SelectOutput
  • Can you add documentation of SelectTopK in a follow-up?

@rusty1s rusty1s changed the title Refactor TopkPooling Refactor TopkPooling into SelectTopK May 8, 2023
@rusty1s rusty1s enabled auto-merge (squash) May 8, 2023 09:56
@rusty1s rusty1s merged commit a43fc56 into master May 8, 2023
@rusty1s rusty1s deleted the topkpool_update branch May 8, 2023 09:59
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.

2 participants