dgl icon indicating copy to clipboard operation
dgl copied to clipboard
verified publisher icon dmlc

[Feature] Added exclude_self and output_batch to knn graph construction (Issues #4323 #4316)

Open ndickson-nvidia opened this issue 3 years ago • 3 comments

Description

  • Added exclude_self and output_batch options to knn_graph and segmented_knn_graph, and their wrappers, dgl.nn.KNNGraph and dgl.nn.SegmentedKNNGraph for Issue #4316
  • Updated out-of-date comments on remove_edges and remove_self_loop, since they now preserve batch information as of PR #3119 , for Issue #4323
  • Added test cases to test_remove_selfloop and test_knn_cuda

Checklist

Please feel free to remove inapplicable items for your PR.

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

ndickson-nvidia avatar Aug 11 '22 18:08 ndickson-nvidia

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

dgl-bot avatar Aug 11 '22 18:08 dgl-bot

Commit ID: 3af4388419bd7f5660cb778d4d46288c115daab1

Build ID: 1

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot avatar Aug 11 '22 19:08 dgl-bot

Commit ID: ff6efb41bca4f0ecc247e62b48e453be3e8a7f28

Build ID: 2

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot avatar Aug 11 '22 23:08 dgl-bot

Commit ID: 8ed2f0d1d625410c525a5764bc5f51d227fae688

Build ID: 3

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

Report path: link

Full logs path: link

dgl-bot avatar Aug 25 '22 16:08 dgl-bot

Commit ID: eed0d9879e2f3763a4d48afa033b0c724557f0f2

Build ID: 4

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot avatar Aug 26 '22 01:08 dgl-bot

Commit ID: c65ebba2bfc6d0c4dcf02105d2cd68d61e6657c7

Build ID: 5

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot avatar Aug 26 '22 20:08 dgl-bot

One question for the team. Is it possible to further reduce the number of new arguments? For example, can we omit output_batch and always assign batch information to the output when the input x is 3D? I'm just pushing everyone to think hard if the current one is a minimal design for user.

jermainewang avatar Aug 30 '22 05:08 jermainewang

can we omit output_batch and always assign batch information to the output when the input x is 3D?

I'd be happy to omit both output_batch and exclude_self (by having it always exclude self-loop edges), if people would be okay with that. I added them defaulting to False to avoid breaking compatibility.

ndickson-nvidia avatar Aug 30 '22 14:08 ndickson-nvidia

After the weekly sync we agreed that the batch information (output_batch) can be omitted and we can go straight to set the batch information, since it would not break existing code. We will keep exclude_self though.

BarclayII avatar Sep 01 '22 01:09 BarclayII

the batch information (output_batch) can be omitted ... We will keep exclude_self though.

Done. Thanks!

ndickson-nvidia avatar Sep 01 '22 21:09 ndickson-nvidia

Commit ID: 0b9e9c2d6d20dd6f80bb617faa9d68baa39f8b16

Build ID: 9

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

Rhett-Ying avatar Sep 04 '22 05:09 Rhett-Ying

Commit ID: None

Build ID: 10

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot avatar Sep 06 '22 02:09 dgl-bot

Commit ID: d8c808bdcb7e32235ea5dce92b5a60c115629797

Build ID: 11

Status: ✅ CI test succeeded

Report path: link

Full logs path: link

dgl-bot avatar Sep 06 '22 07:09 dgl-bot