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

edge_dim added as argument to GATv2Conv #310

Merged
merged 13 commits into from
Nov 27, 2024

Conversation

allaffa
Copy link
Collaborator

@allaffa allaffa commented Nov 26, 2024

This PR applies two major changes:

  • allows to pass edge_dim as argument to GATStack because the original PyG implementation allows to use edge attributes GATv2Conv
  • parametrizes the examples qm9, md17, and LennardJones over all the message passing layers

These changes are crucial to support the future integration go graph transformer architectures and allow a robust testing of those capabilities that should flexibly switch across all message passing layers.

@allaffa allaffa added the enhancement New feature or request label Nov 26, 2024
@allaffa allaffa requested a review from ArCho48 November 26, 2024 03:51
@allaffa allaffa self-assigned this Nov 26, 2024
ArCho48

This comment was marked as resolved.

@RylieWeaver
Copy link
Collaborator

RylieWeaver commented Nov 27, 2024

@allaffa @ArCho48

Max, the fixes you put in place look good to me. For brevity here's my understanding:

It looks like you identified the following Errors and Fixes:
(1) Some MP-layers may not inherently support the existence of positional gradients. In this case, you exclude them from the LJ-example. This fixed the errors with: PNA, SAGE, GIN, GAT, MFC
(2) The examples did not allow you to choose the MP-layer via a command-line-argument. In this case, you added this optionality. This fixed the errors with PNAPlus, PNAEq

Additionally, I added the following in my commits:

(1) Fixes to do with object device placement during degree aggregation in distributed data processing and message-passing. This was not causing errors in only CPU-environments like these tests, but would cause errors in GPU environments and was identified when testing on the DGX.
(2) Squeezed the energy predictions in Base to to avoid the shape mismatch warning.
(3) Add a comment explaining which types of MP-layers are allowed in the grad_energy example in test_examples.py
(4) Added MACE to the grad_energy example because it will propagate a positional gradient.

Lastly, I wasn't able to reproduce the following warning to fix. Do we still want to tackle it?:

  • /home/runner/work/HydraGNN/HydraGNN/hydragnn/utils/model/model.py:178: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).

@allaffa
Copy link
Collaborator Author

allaffa commented Nov 27, 2024

@allaffa @ArCho48

Max, the fixes you put in place look good to me. For brevity here's my understanding:

It looks like you identified the following Errors and Fixes: (1) Some MP-layers may not inherently support the existence of positional gradients. In this case, you exclude them from the LJ-example. This fixed the errors with: PNA, SAGE, GIN, GAT, MFC (2) The examples did not allow you to choose the MP-layer via a command-line-argument. In this case, you added this optionality. This fixed the errors with PNAPlus, PNAEq

Additionally, I added the following in my commits:

(1) Fixes to do with object device placement during degree aggregation in distributed data processing and message-passing. This was not causing errors in only CPU-environments like these tests, but would cause errors in GPU environments and was identified when testing on the DGX. (2) Squeezed the energy predictions in Base to to avoid the shape mismatch warning. (3) Add a comment explaining which types of MP-layers are allowed in the grad_energy example in test_examples.py (4) Added MACE to the grad_energy example because it will propagate a positional gradient.

Lastly, I wasn't able to reproduce the following warning to fix. Do we still want to tackle it?:

  • /home/runner/work/HydraGNN/HydraGNN/hydragnn/utils/model/model.py:178: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).

@RylieWeaver
Thank you for helping me out in this PR.

@allaffa allaffa merged commit b935c88 into ORNL:main Nov 27, 2024
2 checks passed
@allaffa allaffa deleted the gatv2conv_edge_features branch November 27, 2024 17:26
@ArCho48
Copy link
Collaborator

ArCho48 commented Nov 27, 2024 via email

RylieWeaver added a commit to RylieWeaver/HydraGNN that referenced this pull request Dec 28, 2024
* edge_dim added as argument to GATv2Conv

* GAT added in edge_models

* added model_type as optional imput argument to qm9 and md17 examples

* edge_dim passed into GATv2Conv stack inside create method

* architectural arguments added to vectoroutput CI test

* qnum_samples variable moved outside main function scope in qm9 example

* SAGE, GIN, anFC removed from examples where there are edge features

* Correct management of node degree for on GPUs

* split examples test based on whether thee model needs to use data.pos or not

* model_type overwrite in config moved to right location in the code

* comment for allowed stacks in LJ force_grad

* Add MACE to test

* black formatting

---------

Co-authored-by: Rylie Weaver <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants