[WIP] Add PyG-based GAT implementation. #67
Conversation
There was a problem hiding this comment.
Looks good but one more fundamental question. Have we validated the scientific correctness and performance of the new model? If we still need to run these kind of experiments I would suggest we keep them side-by-side as two different classes and then remove one?
In part this is motivated by the effort needed to make sure this works for upcoming ASAP ML deployments
| add_self_loop=True, | ||
| node_featurizer=CanonicalAtomFeaturizer(), | ||
| )(smiles) | ||
| mol = Chem.MolFromSmiles(smiles) |
There was a problem hiding this comment.
Need a convenience function to do this easily for user, easy to mess up.
There was a problem hiding this comment.
I've added one in asapdiscovery for us to use, but since there's no one right way to featurize a molecule I didn't want to add anything opinionated in here
|
|
||
| Default values here are the default values given in DGL-LifeSci. | ||
| Class for constructing a GAT ML model. Default values here are based on the values | ||
| in DGL-LifeSci. |
There was a problem hiding this comment.
the defaults are still based on the defaults in that package, even though we're not using their code anymore
| ) | ||
|
|
||
| return {"g": g, "smiles": smiles} | ||
| mol = Chem.MolFromSmiles(smiles) |
There was a problem hiding this comment.
Same here on convenience func
|
yep that's a good point. I'll add the DGL version back in alongside the PyG version for now |
Switch from using the DGL implementation of GAT to the PyG version. Note that this will break things as the expected input type will change. Closes #59.