-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
DimeNet++
implementation
#4432
DimeNet++
implementation
#4432
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4432 +/- ##
==========================================
- Coverage 82.98% 82.33% -0.65%
==========================================
Files 319 319
Lines 16968 17117 +149
==========================================
+ Hits 14081 14094 +13
- Misses 2887 3023 +136
Continue to review full report at Codecov.
|
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.
Thanks for the PR. Can we add a basic test for DimeNetPlusPlus
? Also I am interested in whether it is possible to make use of pre-trained weights for this model as well (similar to what we do for DimeNet
), see here.
Sure, I will look into using pre-trained weights and also add tests. |
02d5798
to
79a6347
Compare
@rusty1s regarding tests for DimeNetPlusPlus, I have added a test for checking output size and another one test for checking whether the model is training well or not - test_overfit. The downside of test_overfit is that it is sometimes flaky but otherwise it is a good test for checking whether the model is able to train or not. |
8b79778
to
91c63ec
Compare
Thank you @arunppsg! I am currently on vacation, will have a final look in the next week. |
The failures in testing are due to not having |
DimeNet model, when used with OutputPPBlock has an additional layer lin_up for which we don't have pre-trained weights
@rusty1s added the necessary requirements. |
Thanks @arunppsg. I think I fixed the dependency issue during testing. I also added a
Do you know what's the reason for this? |
Not sure about it, the results are far off from the dimenet++ paper. Can we create an issue and track it separately? |
This sounds good to me. I will try to look into this as well. |
Thank you! |
In this PR, I implemented DimeNet++ by subclassing DimeNet. The
InteractionPPBlock
andOutputPPBlock
was based on this implementation.Also, I depreciated
OutputBlock
, thereby DimeNet model will also be usingOutputPPBlock
.OutputPPBlock
allows up-projection and down-projection of embeddings, thereby removing information bottlenecks.Fixes #4427