Skip to content

Conversation

@puririshi98
Copy link
Contributor

@puririshi98 puririshi98 commented Oct 23, 2023

Epoch: 20, Step: 1000, Loss: 0.8472, Train Acc: 72.31%, Most Recent Step Time: 0.1662s
Epoch: 20, Loss: 0.8822, Train Acc:72.31%, Average Step Time: 0.1624s
Validation Accuracy: 68.7001%
Test Accuracy: 68.6972%

PR finally ready, test accuracy is on par with claimed accuracies in OGB repo

@puririshi98
Copy link
Contributor Author

@akihironitta lmk if theres anything else needed to merge this, we have been using this internally and in our containers for some time now, it would be great for all PyG users to have access.
snap-stanford/ogb#449
this also needs merging

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@puririshi98 It would also be nice to add a quick docstring of the model args.

Also, would it make sense to have this script in examples/multi_gpu/ and add examples/multi_gpu/README.md briefly describing what each of the multi-gpu scripts (that you recently added) does and/or how each example differs from each other?

@puririshi98
Copy link
Contributor Author

@akihironitta addressd, ur comments, lmk if you have any further review requests

@puririshi98
Copy link
Contributor Author

@rusty1s @akihironitta lmk if there is anything else i would need to get this merge ready

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

Looks great! I'll just keep my review a "comment" as I'd like to run this in my env, too :) Feel free to ask Matthias to merge if I'm too slow ;)

@puririshi98
Copy link
Contributor Author

@akihironitta thanks for the review, no rush, please merge once your ready.

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.

Nice. Note that you also need to call acc.reset() in order to avoid sharing metrics between training/validation.

@puririshi98 puririshi98 merged commit 9b660ac into master Feb 29, 2024
@puririshi98 puririshi98 deleted the mag240m-example branch February 29, 2024 13:52
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.

4 participants