AstraZeneca / chemicalx

A PyTorch and TorchDrug based deep learning library for drug pair scoring. (KDD 2022)
https://chemicalx.readthedocs.io
Apache License 2.0
700 stars 89 forks source link

Implement DeepDrug model #68

Closed kajocina closed 2 years ago

kajocina commented 2 years ago

Closes #14

Added the DeepDrug model, trying to copy as much as possible from their approach. The paper didn't use any context features, but I implemented the model so that it could be used in both ways. I ran the model with and without context feats on DrugCombDB with around 0.76 AUROC (no context features led to a minor drop in AUROC).

Provided an example that runs the model and a unit test which tests both the context and no-context modes of this model.

cthoyt commented 2 years ago

@kajocina please pass CI before we review this PR. You can either read the results in the GitHub browser or run tox locally to see all issues that need to be fixed

cthoyt commented 2 years ago

I would suggest implementing this class only to have the functionality as originally described, then to consider subclassing it to have your additional functionality (adding in the context features may actually make it the same as one of the other models)

kajocina commented 2 years ago

@kajocina please pass CI before we review this PR. You can either read the results in the GitHub browser or run tox locally to see all issues that need to be fixed

@cthoyt I am getting CI errors from code in our tox dependencies so I can't really fix those, e.g.:

lint/bin/activate_this.py:28: error: "str" has no attribute "decode"; maybe "encode"?
lint/bin/activate_this.py:31: error: Module has no attribute "real_prefix"
Found 2 errors in 1 file (checked 39 source files)

I did fix tox issues related to my commits now.

cthoyt commented 2 years ago

@kajocina you can try running tox where it automatically rebuilds the virtualenvs with tox -r. If that's not going well you can even consider reinstalling tox itself. However, the CI run on GitHub actions still shows issues in the lint and documentation tasks. Please look into those

kajocina commented 2 years ago

@cthoyt thanks for the tip, rebuilt tox and works ok, local CI was green now. I added the wrapper as you suggested, but it didnt really decrease the number of lines of code in total, only the number of assignments performed.

cthoyt commented 2 years ago

@kajocina the goal was to make the model more legibile. I will demonstrate by pushing to your branch - see 1c60cae .

codecov-commenter commented 2 years ago

Codecov Report

Merging #68 (e459ba6) into main (5449f96) will increase coverage by 0.14%. The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   93.87%   94.01%   +0.14%     
==========================================
  Files          29       29              
  Lines         832      869      +37     
==========================================
+ Hits          781      817      +36     
- Misses         51       52       +1     
Impacted Files Coverage Δ
chemicalx/models/deepdrug.py 96.77% <96.66%> (-3.23%) :arrow_down:
tests/unit/test_models.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5449f96...e459ba6. Read the comment docs.

cthoyt commented 2 years ago

Now that we have all of the implementations, I can see some interesting abstractions we can provide!