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

[Documentation] make documentation of GATConv and GATv2Conv more explicit and accurate #8201

Merged
merged 7 commits into from
Oct 21, 2023

Conversation

SZiesche
Copy link
Contributor

What?

I explicitly named the different \Thetas used in the different layers by an index s and t (like it is already done in the docs when talking about input sizes). I did the same for a in the GATconv layer. Finally removed the unclear || operator and just wrote out the few additions.

Why?

To solve the issues discussed in #8200

Testing?

No tests needed, as only documentation was changed.

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #8201 (3b7ff8c) into master (7085a3a) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 3b7ff8c differs from pull request most recent head ae15bc1. Consider uploading reports for the commit ae15bc1 to get more accurate results

@@            Coverage Diff             @@
##           master    #8201      +/-   ##
==========================================
- Coverage   87.40%   87.39%   -0.02%     
==========================================
  Files         473      473              
  Lines       28613    28613              
==========================================
- Hits        25010    25006       -4     
- Misses       3603     3607       +4     
Files Coverage Δ
torch_geometric/nn/conv/gat_conv.py 100.00% <ø> (ø)
torch_geometric/nn/conv/gatv2_conv.py 94.73% <ø> (ø)

... and 1 file with indirect coverage changes

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

@EdisonLeeeee EdisonLeeeee changed the title [Documentation] make documentation of GATconv and GATv2conv more explicit and accurate. [Documentation] make documentation of GATconv and GATv2conv more explicit and accurate Oct 16, 2023
@rusty1s rusty1s changed the title [Documentation] make documentation of GATconv and GATv2conv more explicit and accurate [Documentation] make documentation of GATConv and GATv2Conv more explicit and accurate Oct 17, 2023
@SZiesche
Copy link
Contributor Author

ok great, should we make the two failing checks pass? I had a look at the changelog, but the PR doesn't seem to contain something worth mentioning there. The test for previous pytorch version seems not to be failing because of this PR.

How can the stuff be merged in the end?

@rusty1s rusty1s linked an issue Oct 20, 2023 that may be closed by this pull request
@SZiesche
Copy link
Contributor Author

hey guys, I wanted to ask again, who can push the merge button (once this is updated with master) and if we need to make the changelog happy.

@EdisonLeeeee
Copy link
Contributor

@rusty1s would take care of it :)

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.

Thank you, this is indeed much clearer :)

@rusty1s rusty1s enabled auto-merge (squash) October 21, 2023 16:34
@rusty1s rusty1s merged commit 4553ca8 into pyg-team:master Oct 21, 2023
12 of 13 checks passed
@SZiesche
Copy link
Contributor Author

Thank you, this is indeed much clearer :)

It was a pleasure. I always like to share such improvements. Documentation is always a difficuilt point, because the author of the code is often hugely biased and has a large knowledge advantage, which makes it pretty hard for him to write the docs.

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.

GATconv and GATv2conv
3 participants