Model Wrapper for GATPredictor and AttentiveFPPredictor from DGL-LifeSci #2280
Conversation
There was a problem hiding this comment.
Great, glad to see this make its way in! Did a first very quick pass through the PR. There appear to be some indentation issues (not technically wrong, but good to fix for code style).
For the conflict, can you try rebasing against master? You can do this roughly as follows:
git checkout master
git fetch origin
git pull
git checkout my_branch
git rebase origin/master
You can then fix the rebase conflicts manually in an editor then run the following commands
git add docs/models.rst
git rebase --continue
For the Weave/MPNN models, the Weave model is well tested and stable. The MPNN implementation isn't as good I think. If the DGL-Lifesci implementation is tested and robust, swapping out the MPNN backend (without changing any constructor settings so users won't see a difference), might be a good first test. One way to do this might be to add dc.models.pytorch_models.MPNNModel, then over the course of a release deprecate the existing implementation and swap out for the new implementation.
I'll do a more thorough review pass in a bit once @nd-02110114 and @peastman have had a chance to take a look as well
| def forward(self, g): | ||
| """Predict graph labels | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
The indentation here might be off?
There was a problem hiding this comment.
This should be fixed now.
| self_loop: bool = True, | ||
| **kwargs): | ||
| """ | ||
| Parameters |
Thank you. I fixed the conflict using the commands below. git remote add upstream https://github.com/deepchem/deepchem.git
git fetch upstream
git rebase upstream/master
git push origin master |
|
And, can you make another PR for the MPNNModel...? |
Done. |
|
Thank you for your all fixes! |
|
@rbharath @nd-02110114 Thank you for the help. You may go ahead to merge this PR. It seems that the tests for WGAN and PPO still fail from time to time. |
|
@mufeili Thanks for the contribution! Going to go ahead and merge in :) |
This PR adds a model wrapper for GAT and AttentiveFP from DGL-LifeSci. The PR assumes DGL (0.5.0+) and DGL-LifeSci (0.2.5+).
As @nd-02110114 suggested in #2249 , we are replacing the previous implementation of GAT for better robustness.
I have not added the wrappers for Weave and MPNN as deprecating long-existing implementations should be carefully handled.
Once this PR is merged, we may start adding some examples for the MoleculeNet benchmark.
@rbharath @nd-02110114 @peastman This PR is complete and you may start the code review. I also need to bother you to resolve the conflicts.