[Example][Refactor] Refactor RGAT example by chang-l · Pull Request #4530 · dmlc/dgl

chang-l · 2022-09-08T22:34:10Z

Description

To resolve #4411, this PR refactors rgat example according to the golden example #4186.

Checklist

Please feel free to remove inapplicable items for your PR.

The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage
Code is well-documented
To the best of my knowledge, examples are either not affected by this change,
or have been fixed to be compatible with this change
Related issue is referred in this PR
If the PR is for a new model/paper, I've updated the example index here.

Changes

Similar code style as previous refactors

Results

Dataset	Test acc. (before)	Mean	Test acc. (after)	Mean
ogbn-mag	[0.3492 0.3667 0.3543 0.3631 0.3648]	0.3596	[0.3588 0.3457 0.3617 0.3647 0.3852]	0.3632

dgl-bot · 2022-09-08T22:34:38Z

To trigger regression tests:

@dgl-bot run [instance-type] [which tests] [compare-with-branch];
For example: @dgl-bot run g4dn.4xlarge all dmlc/master or @dgl-bot run c5.9xlarge kernel,api dmlc/master

chang-l · 2022-09-08T22:43:41Z

@mufeili As I renamed file, the diff is not there... Sorry for any inconvenience. Pls feel free to let me change it back to ease the review process.

dgl-bot · 2022-09-08T23:21:18Z

Commit ID: 8ee0590

Build ID: 1

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot · 2022-09-08T23:47:52Z

Commit ID: 02f07dc

Build ID: 2

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

mufeili · 2022-09-13T07:36:57Z

@mufeili As I renamed file, the diff is not there... Sorry for any inconvenience. Pls feel free to let me change it back to ease the review process.

I think you can get the benefit of the both by directly renaming the original file rather than create a new file and delete the original file.

chang-l · 2022-09-14T06:10:24Z

I think you can get the benefit of the both by directly renaming the original file rather than create a new file and delete the original file.

Really? I don't know where I did wrong. I tried git mv rgat.py train.py, but it still shows the same file diff in github.

As for alternative, I reformed the commit tree of this PR. Please checkout this commit for review 025cb1f. The other commit is just for file renaming.

dgl-bot · 2022-09-14T07:27:45Z

Commit ID: None

Build ID: 3

Status: ❌ CI test failed in Stage [Declarative: Checkout SCM].

Report path: link

Full logs path: link

dgl-bot · 2022-09-14T07:27:45Z

Commit ID: None

Build ID: 4

Status: ❌ CI test failed in Stage [Declarative: Checkout SCM].

Report path: link

Full logs path: link

dgl-bot · 2022-09-14T07:27:46Z

Commit ID: None

Build ID: 5

Status: ❌ CI test failed in Stage [Declarative: Checkout SCM].

Report path: link

Full logs path: link

dgl-bot · 2022-09-14T09:35:37Z

Commit ID: 6d4060e332d5572b7fbedac622867c703fd43d73

Build ID: 6

Status: ❌ CI test failed in Stage [C++ CPU].

Report path: link

Full logs path: link

mufeili · 2022-09-16T08:14:59Z

I think you can get the benefit of the both by directly renaming the original file rather than create a new file and delete the original file.

Really? I don't know where I did wrong. I tried git mv rgat.py train.py, but it still shows the same file diff in github.

As for alternative, I reformed the commit tree of this PR. Please checkout this commit for review 025cb1f. The other commit is just for file renaming.

Did this happen before? I think you also did something like that before. What if you rename the file via an IDE?

chang-l · 2022-09-20T00:35:57Z

I am not sure... I tried in IDE(vscode) but it still doesn't work. I changed the file name back for now.

dgl-bot · 2022-09-20T00:43:45Z

Commit ID: 5f61165d57bc060ae9efde85a021bd709dfce0ce

Build ID: 8

Status: ❌ CI test failed in Stage [Lint Check].

Report path: link

Full logs path: link

dgl-bot · 2022-09-20T00:43:48Z

Commit ID: 93c4a79

Build ID: 9

Status: ❌ CI test failed in Stage [Lint Check].

Report path: link

Full logs path: link

dgl-bot · 2022-09-20T00:52:25Z

Commit ID: 4f8a3464235e2af3b519521926fa9a5a244cd927

Build ID: 7

Status: ❌ CI test failed in Stage [C++ CPU].

Report path: link

Full logs path: link

dgl-bot · 2022-09-20T01:45:09Z

Commit ID: 26f211f

Build ID: 10

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot · 2022-09-20T02:10:21Z

Commit ID: 56c684d

Build ID: 11

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot · 2022-09-21T22:52:35Z

Commit ID: a2d8e53

Build ID: 13

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

chang-l · 2022-09-21T22:57:21Z

@mufeili
I think the reason Git resists to show rename diff, but file create/deletion, is due to its rename detection mechanism. When the file diff > 50% (default), the git tool will show it as file delete/create, instead of rename. It is changeable though, for example, using git diff -M20%, or git diff --find-renames 20% means consider a delete/add pair to be a rename if more than 20% of the file has NOT changed.
Here are some references:
https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---find-renamesltngt
https://github.com/git/git/blob/142430338477d9d1bb25be66267225fb58498d92/Documentation/gitdiffcore.txt#L123-L129
https://github.com/git/git/blob/36f8e7ed7d72d2ac73743c3c2226cceb29b32156/Documentation/config/diff.txt#L126-L133

mufeili · 2022-10-12T15:32:40Z

@chang-l Cool, thanks for sharing the information

mufeili · 2022-10-12T17:14:13Z

+        val_acc = evaluate(model, val_dataloader, 'Val. ')
+        test_acc = evaluate(model, test_dataloader, 'Test ')
+        print("Epoch {:05d} | Loss {:.4f} | Validation Acc. {:.4f} | Test Acc. {:.4f} "
+              . format(epoch, total_loss / (it + 1), val_acc.item(), test_acc.item()))


We now recommend using f-strings like this.

Thanks. I updated accordingly.

mufeili · 2022-10-12T17:14:41Z

Sorry for the late review. It looks great in general and I've left some minor comments.

dgl-bot · 2022-10-17T19:41:05Z

Commit ID: 80e5193

Build ID: 14

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot · 2022-10-18T06:45:45Z

Commit ID: d145458

Build ID: 15

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

chang-l requested a review from mufeili September 8, 2022 22:43

chang-l force-pushed the refactor-rgat-example branch from 0ea2c73 to e038798 Compare September 14, 2022 06:06

chang-l force-pushed the refactor-rgat-example branch from 93c4a79 to 26f211f Compare September 20, 2022 00:33

chang-l force-pushed the refactor-rgat-example branch from 56c684d to 575203b Compare September 21, 2022 21:41

chang-l added 2 commits September 21, 2022 15:05

RGAT refactor

68814b5

File rename

a2d8e53

chang-l force-pushed the refactor-rgat-example branch from 575203b to a2d8e53 Compare September 21, 2022 22:08

mufeili reviewed Oct 12, 2022

View reviewed changes

Comment thread examples/pytorch/rgat/README.md Outdated

mufeili reviewed Oct 12, 2022

View reviewed changes

Comment thread examples/pytorch/rgat/README.md Outdated

mufeili reviewed Oct 12, 2022

View reviewed changes

Comment thread examples/pytorch/rgat/train.py Outdated

mufeili reviewed Oct 12, 2022

View reviewed changes

chang-l added 2 commits October 17, 2022 11:57

Address comments

4d6d611

Merge branch 'master' into refactor-rgat-example

80e5193

Merge branch 'master' into refactor-rgat-example

d145458

mufeili approved these changes Oct 18, 2022

View reviewed changes

mufeili merged commit 98792a8 into dmlc:master Oct 18, 2022

chang-l deleted the refactor-rgat-example branch October 19, 2022 05:15

Conversation

chang-l commented Sep 8, 2022

Description

Checklist

Changes

Results

Uh oh!

dgl-bot commented Sep 8, 2022

Uh oh!

chang-l commented Sep 8, 2022

Uh oh!

dgl-bot commented Sep 8, 2022

Uh oh!

dgl-bot commented Sep 8, 2022

Uh oh!

mufeili commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chang-l commented Sep 14, 2022

Uh oh!

dgl-bot commented Sep 14, 2022

Uh oh!

dgl-bot commented Sep 14, 2022

Uh oh!

dgl-bot commented Sep 14, 2022

Uh oh!

dgl-bot commented Sep 14, 2022

Uh oh!

mufeili commented Sep 16, 2022

Uh oh!

chang-l commented Sep 20, 2022

Uh oh!

dgl-bot commented Sep 20, 2022

Uh oh!

dgl-bot commented Sep 20, 2022

Uh oh!

dgl-bot commented Sep 20, 2022

Uh oh!

dgl-bot commented Sep 20, 2022

Uh oh!

dgl-bot commented Sep 20, 2022

Uh oh!

dgl-bot commented Sep 21, 2022

Uh oh!

chang-l commented Sep 21, 2022

Uh oh!

mufeili commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mufeili Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

chang-l Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

mufeili commented Oct 12, 2022

Uh oh!

dgl-bot commented Oct 17, 2022

Uh oh!

dgl-bot commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mufeili commented Sep 13, 2022 •

edited

Loading