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

Tutorial on nn.aggr package #5191

Merged
merged 9 commits into from
Aug 12, 2022
Merged

Tutorial on nn.aggr package #5191

merged 9 commits into from
Aug 12, 2022

Conversation

rusty1s
Copy link
Member

@rusty1s rusty1s commented Aug 11, 2022

No description provided.

@rusty1s rusty1s requested a review from mananshah99 August 11, 2022 23:41
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #5191 (82879b6) into master (7469ede) will not change coverage.
The diff coverage is n/a.

❗ Current head 82879b6 differs from pull request most recent head df8ecfd. Consider uploading reports for the commit df8ecfd to get more accurate results

@@           Coverage Diff           @@
##           master    #5191   +/-   ##
=======================================
  Coverage   83.05%   83.05%           
=======================================
  Files         335      335           
  Lines       18485    18485           
=======================================
  Hits        15353    15353           
  Misses       3132     3132           
Impacted Files Coverage Δ
torch_geometric/nn/conv/sage_conv.py 96.77% <ø> (ø)

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

Copy link
Contributor

@Padarn Padarn left a comment

Choose a reason for hiding this comment

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

Some very minor nits, but overall looks great! LGTM

docs/source/modules/nn.rst Outdated Show resolved Hide resolved

output = mean_aggr(x, index) # Output shape: [100, 64]

Notably, all aggregations share the same set of forward arguments, as described in detail in :meth:`torch_geometric.nn.aggr.Aggregation.forward`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to caveat here that there is not yet support for these arguments across all aggregations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I think we should do this in the doc-strings of the individual modules, using a warning box for example. Let me know if you want to add this in.

Copy link
Contributor

@mananshah99 mananshah99 left a comment

Choose a reason for hiding this comment

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

Nice :)

docs/source/modules/nn.rst Outdated Show resolved Hide resolved
docs/source/modules/nn.rst Show resolved Hide resolved
rusty1s and others added 3 commits August 12, 2022 06:56
Co-authored-by: Padarn Wilson <padarn.wilson@grabtaxi.com>
Co-authored-by: Manan Shah <manan.shah.777@gmail.com>
@rusty1s rusty1s enabled auto-merge (squash) August 12, 2022 05:37
@rusty1s rusty1s merged commit a00f2eb into master Aug 12, 2022
@rusty1s rusty1s deleted the aggr_doc branch August 12, 2022 05:40
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.

3 participants