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

Make generate_graph from GraphModelMixin standalone #836

Closed
wants to merge 7 commits into from

Conversation

EricZQu
Copy link

@EricZQu EricZQu commented Sep 6, 2024

Changes:

  • Move GraphModelMixin.generate_graph to common.utils, and backwards compatible
  • Make the input logic for generate_graph clear (now if the input arguments are different from model attributes, unexpected behaviors could happen)

Enhancements:

  • Enable a standalone version of generate_graph for dev (don't need to build a whole model)
  • Don't require model to have explicit attributes like use_pbc_single

@misko misko self-requested a review September 9, 2024 23:11
misko
misko previously approved these changes Sep 9, 2024
Copy link
Collaborator

@misko misko 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! LGTM!

src/fairchem/core/models/base.py Outdated Show resolved Hide resolved
@misko misko self-requested a review September 10, 2024 17:57
misko
misko previously approved these changes Sep 10, 2024
@@ -779,6 +780,114 @@ def radius_graph_pbc(
return edge_index, unit_cell, num_neighbors_image


def generate_graph_standalone(
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just call it generate_graph? or is there a name clash somewhere that I missed?

Copy link
Author

Choose a reason for hiding this comment

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

Agree and fixed!

@misko misko self-requested a review September 16, 2024 16:15
Copy link

This PR has been marked as stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale label Oct 17, 2024
@github-actions github-actions bot closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants