-
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
Adding more grover layers #3277
Conversation
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.
Mostly minor comments here requesting improvements to the docs
the number of attention heads. | ||
atom_embedding_output_type: str | ||
the type of output aggregation after message passing. | ||
atom_messages: True False |
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.
Is the formatting here messed up? It looks strange
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 it
class GroverBondVocabPredictor(nn.Module): | ||
"""Layer for learning contextual information for bonds. | ||
|
||
The layer is used in `Grover <https://drug.ai.tencent.com/publications/GROVER.pdf>`_ |
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 References
section and move the link there?
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 reference section
"""Grover Atom Vocabulary Prediction Module. | ||
|
||
The GroverAtomVocabPredictor module is used for predicting atom-vocabulary | ||
for the self-supervision task in `Grover. <https://drug.ai.tencent.com/publications/GROVER.pdf>`_ |
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.
Same comments as before. Can you add References
and can you add a usage example?
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.
One minor fix needed on the docstring and we should be good to go
in_features: int, default: 128 | ||
Input feature size of bond embeddings. | ||
|
||
Example |
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.
This should be in the main docstring above and not in the init docstring
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 in once CI looks good
Pull Request Template
Description
I am breaking down #3272 into two pull requests. Here is the part one, containing some layers in the grover pretrain model.
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