-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
extracting attributes for grover model from batch graph #3312
Conversation
0a638f5
to
ff99735
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design in general looks good here. Some minor comments and questions below that should be straightforward to address
deepchem/feat/graph_data.py
Outdated
@@ -336,6 +336,7 @@ def __init__(self, graph_list: Sequence[GraphData]): | |||
]) | |||
|
|||
# graph_index indicates which nodes belong to which graph | |||
# TODO should we rename this as node_index? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove TODO before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed TODO but a question I have is should we rename graph_index
as node_index
because I think node_index
is more descriptive. The variable here stores a mapping for every node and the graph to which the node belongs to.
deepchem/utils/grover.py
Outdated
|
||
|
||
def _get_a2b(n_atoms, edge_index): | ||
"""a2b is a mapping between atoms and their incoming bonds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a Parameters
section here documenting the types of the argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
deepchem/utils/grover.py
Outdated
def extract_grover_attributes(molgraph: BatchGraphData): | ||
"""Utility to extract grover attributes for grover model | ||
|
||
Parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Parameters (missing "s")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to merge once CI is clear
@gusty1g Looks like some mypy errors which should be fixed before merge |
62905d0
to
02e98a0
Compare
02e98a0
to
b792487
Compare
Description
Related to #3296
This pull request proposes a new approach to extract required attributes for a grover model from a batched molecular graph.
Type of change
Please check the option that is related to your PR.
Checklist
yapf -i <modified file>
and check no errors (yapf version must be 0.32.0)mypy -p deepchem
and check no errorsflake8 <modified file> --count
and check no errorspython -m doctest <modified file>
and check no errors