Skip to content
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 DrugBank DDI and Two Sides. #48

Merged
merged 18 commits into from
Jan 17, 2022
Merged

Conversation

benedekrozemberczki
Copy link
Contributor

@benedekrozemberczki benedekrozemberczki commented Jan 17, 2022

Summary

  • Adding DrugBank DDI and TwoSides.

  • Code passes all tests

  • Unit tests provided for these changes

  • Documentation and docstrings added for these changes

Changes

  • Adds the DrugBank DDI and TwoSides Datasets.
  • Adds the loaders.
  • Adds tests.
  • Adds documentation of data cleaning.
  • Adds the data cleaning scripts.

@cthoyt
Copy link
Collaborator

cthoyt commented Jan 17, 2022

I’d highly request provenance information on how these new datasets were constructed. Were they automatically downloaded from Some external repo? Was processing done to them?

@benedekrozemberczki
Copy link
Contributor Author

Yes, @cthoyt I will do that in a moment.

@benedekrozemberczki
Copy link
Contributor Author

There will be a whole Appendix section about this in the paper.

@cthoyt
Copy link
Collaborator

cthoyt commented Jan 17, 2022

While you’re thinking about it maybe also consider doing the same for the previous two datasets as well :)

@benedekrozemberczki
Copy link
Contributor Author

@cthoyt How about a dataset preprocessing section in the documentation?

@cthoyt
Copy link
Collaborator

cthoyt commented Jan 17, 2022

Tbh the only important documentation of data preprocessing to me is code that can exactly reproduce it. Let’s start there and backfill prose-based documentation if there are any places where it can’t be better documented in code itself

@benedekrozemberczki
Copy link
Contributor Author

Added the cleaning scripts.

@benedekrozemberczki benedekrozemberczki merged commit 9f61eed into main Jan 17, 2022
@benedekrozemberczki benedekrozemberczki deleted the deepddi-twosides branch January 17, 2022 21:42
@cthoyt
Copy link
Collaborator

cthoyt commented Jan 17, 2022

It appears you merged the branch with failing tests. This shouldn’t be allowed/possible - the solution is to add some branch protection rules in the settings for the repository

@benedekrozemberczki
Copy link
Contributor Author

Does it require changes to Github actions?
Screenshot 2022-01-17 at 22 19 47

@cthoyt
Copy link
Collaborator

cthoyt commented Jan 17, 2022

Nope that looks right!

@cthoyt cthoyt added the dataset label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants