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.

Sign up for GitHub

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

Jump to bottom

Incorporating Equivariance #194

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Incorporating Equivariance #194

merged 3 commits into from
Oct 31, 2023

Conversation

JustinBakerMath
Copy link
Collaborator

A reformatting of the Base class and the inherited classes to support equivariance through updating positional data (data.pos) in each graph convolution.

@JustinBakerMath JustinBakerMath added the enhancement New feature or request label Oct 23, 2023
allaffa
allaffa reviewed Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@allaffa allaffa left a comment

Choose a reason for hiding this comment

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

Just have one little questions.

@allaffa allaffa self-requested a review October 25, 2023 17:00
pzhanggit
pzhanggit reviewed Oct 25, 2023
View reviewed changes
Copy link
Collaborator

@pzhanggit pzhanggit left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. See my comments below.

hydragnn/models/GINStack.py Outdated Show resolved Hide resolved
hydragnn/models/MFCStack.py Outdated Show resolved Hide resolved
hydragnn/models/SAGEStack.py Outdated Show resolved Hide resolved
hydragnn/models/SCFStack.py Show resolved Hide resolved
hydragnn/models/SCFStack.py
return radial, coord_diff


def unsorted_segment_mean(data, segment_ids, num_segments):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same functions are defined in EGCLStack.py. Could we reuse those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There currently aren't any clear utility files to put common methods in, but perhaps we can add a utils.py file in hydragnn/models/ @allaffa .

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JustinBakerMath
We have a directory called hydragnn/utils that is already dedicated to all the possible utilities.
Inside of this directory, we have a file called models.py that collects all the utilities that support the definition of a model. Why don't we use that file?

hydragnn/models/SCFStack.py Show resolved Hide resolved
@allaffa
Copy link
Collaborator

allaffa commented Oct 30, 2023

@JustinBakerMath we need to resolve some conflicts with the new version of the branch since I merged PR#195 (activation functions). Let me know if you want me to take care of the rebasing myself. Hopefully, this will be pretty straightforward to take care of.

@jychoi-hpc
Copy link
Member

I was able to run a few examples without errors. But, I am not sure if I really tested this new equivariance feature. I am just wondering if there is any example in the list of our examples to check this new feature. I am thinking simply turning on "equivariance: true" and changing "model_type: EGNN or SchNet".

Otherwise, it looks good. I ran unit tests (no mpi) and they went ok in my local machine.

@allaffa
Copy link
Collaborator

allaffa commented Oct 30, 2023

I was able to run a few examples without errors. But, I am not sure if I really tested this new equivariance feature. I am just wondering if there is any example in the list of our examples to check this new feature. I am thinking simply turning on "equivariance: true" and changing "model_type: EGNN or SchNet".

Otherwise, it looks good. I ran unit tests (no mpi) and they went ok in my local machine.

@jychoi-hpc
yes, we can use the equivariant for the QM9 dataset.

@JustinBakerMath
Copy link
Collaborator Author

@JustinBakerMath we need to resolve some conflicts with the new version of the branch since I merged PR#195 (activation functions). Let me know if you want me to take care of the rebasing myself. Hopefully, this will be pretty straightforward to take care of.

I have now merged the changes to the activation functions to the changes for equivariance.

@allaffa allaffa requested a review from pzhanggit October 31, 2023 01:28
@allaffa
Copy link
Collaborator

allaffa commented Oct 31, 2023

@JustinBakerMath we need to resolve some conflicts with the new version of the branch since I merged PR#195 (activation functions). Let me know if you want me to take care of the rebasing myself. Hopefully, this will be pretty straightforward to take care of.

I have now merged the changes to the activation functions to the changes for equivariance.

@JustinBakerMath Thanks. I will run the equivariance with the QM9 dataset. @jychoi-hpc would you mind doing the same?
The best example for this would be the GDB-9-Ex dataset. However, the GDB-9-Ex dataset is not updated in the current version of he main branch. So I think that using the standard QM9 is the best thing to do.

pzhanggit
pzhanggit approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@pzhanggit pzhanggit left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@allaffa
Copy link
Collaborator

allaffa commented Oct 31, 2023

Looks good to me, thanks!

@pzhanggit, thanks! I just asked @JustinBakerMath to move unsorted_segment_mean into hydragnn/utils/models.py.

@allaffa
Copy link
Collaborator

allaffa commented Oct 31, 2023

I was able to run a few examples without errors. But, I am not sure if I really tested this new equivariance feature. I am just wondering if there is any example in the list of our examples to check this new feature. I am thinking simply turning on "equivariance: true" and changing "model_type: EGNN or SchNet".
Otherwise, it looks good. I ran unit tests (no mpi) and they went ok in my local machine.

@jychoi-hpc yes, we can use the equivariant for the QM9 dataset.

@jychoi-hpc I ran the QM9 example in debug mode using PyCharm with model_type = "EGNN" and equivariance = True. I could see the equivariant message passing being activated.

@jychoi-hpc
Copy link
Member

Great. I was able to run too with model_type = "EGNN" and equivariance = True for QM9. This PR looks good to me.

@allaffa allaffa merged commit 9c1341d into ORNL:main Oct 31, 2023
2 checks passed
RylieWeaver pushed a commit to RylieWeaver/HydraGNN that referenced this pull request Oct 17, 2024
@JustinBakerMath
Incorporating Equivariance (ORNL#194)
438ce56 
* equivariant reformat

* incorporating equivariance with edge_attr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allaffa allaffa allaffa approved these changes

@pzhanggit pzhanggit pzhanggit approved these changes

@jychoi-hpc jychoi-hpc jychoi-hpc approved these changes

Assignees

@allaffa allaffa

@JustinBakerMath JustinBakerMath

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JustinBakerMath @allaffa @jychoi-hpc @pzhanggit