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

add discussion in interpretability section and updates to molecular design section and discussion sections (issues with previous PR fixed) #988

Merged
merged 24 commits into from
Aug 9, 2020

Conversation

delton137
Copy link
Contributor

If there are other areas of the paper you think I should work on, please let me know and I'll see what I can do.

@AppVeyorBot
Copy link

AppVeyor build 1.0.23 for commit b77c8a1 by @delton137 is now complete. The rendered manuscript from this build is temporarily available for download at:

@cgreene
Copy link
Member

cgreene commented Mar 5, 2020

Could you discuss how you've addressed @akundaje's points in #986? @akundaje - did you want to review this? It sounded like you wanted to be involved. Thanks!

@cgreene cgreene mentioned this pull request Mar 5, 2020
@akundaje
Copy link
Contributor

akundaje commented Mar 5, 2020 via email

* Experiment with v2 author contribution whitespace

* Remove 2019 dates from readme

* Add new v1 author contribution category for dhimmel
Match the contribution listed in metadata.yaml

* Typo in documentation

* Standardize capitalization and minor rephrasing

* One more capitalization change

* Add whitespace
@agitter
Copy link
Collaborator

agitter commented Mar 16, 2020

@stephenra you had started to review the molecule design edits in #985. Do you want to look over this version, or should I?

@delton137, #991 updated Manubot and switched to a new form of citation tags. We can help you with instructions for resolving the merge conflict once we finish reviewing the other content.

@stephenra
Copy link
Collaborator

@stephenra you had started to review the molecule design edits in #985. Do you want to look over this version, or should I?

@agitter Sure, happy to take a look.

cbrueffer and others added 8 commits April 11, 2020 16:15
* Fix typos and a spurious word.

* Harmonize spelling of word2vec.

word2vec was already the predominant spelling in this repository and is
the version used in the original word2vec code repository.

* Update content/06.discussion.md

Co-Authored-By: Anthony Gitter <[email protected]>

Co-authored-by: Anthony Gitter <[email protected]>
@delton137
Copy link
Contributor Author

Hi, I'm sorry I've been out of touch. I just resolved the merge conflicts between the current version and my forked version and pushed the result to https://github.com/delton137/deep-review. Hopefully that helps .. it's saying "no conflicts now"

If there's anything else I can do, please let me know. I'd be happy to do some proofreading and editing, if needed.

@AppVeyorBot
Copy link

AppVeyor build 1.0.86 for commit 9d190e4 by @delton137 is now complete. The rendered manuscript from this build is temporarily available for download at:

@agitter
Copy link
Collaborator

agitter commented Apr 20, 2020

@delton137 I don't think there is anything else needed on your side right now. This pull request is only waiting for a few of us to review and provide comments.

@delton137
Copy link
Contributor Author

Hey, I hate to bug you guys, but I was wondering what the status of this project is and when my edits might show up on https://greenelab.github.io/deep-review/. Thanks. Happy to help if I can.

@delton137
Copy link
Contributor Author

delton137 commented Jul 29, 2020

Thank you for your remarks and quick action on this. I agree moving my edits on interpretability techniques to a different PR makes sense. I also had a few things I wanted to add/change there, also.

You'll have to forgive me because I'm a bit confused as to the best way to submit my PR. For that new PR should I get my repo up to speed with the master?

I've been working off commit #986 from the master. When I try to pull I get a merge error. Should I try to merge in the stuff from the latest version and then make the PR?

_"Can you please confirm these are the only files you updated:

content/05.treat.md
content/06.discussion.md
content/citation-tags.tsv
content/manual-references.json"_

-- Yes, that is right.. however there are some other changes showing up in my PR here under "files" which seem like changes you or one of the main authors made.. which is odd.

@agitter
Copy link
Collaborator

agitter commented Aug 1, 2020

Regarding the branches and PRs, I recommend you continue working on delton137:master to address these comments. Let's not try to merge with greenelab:master yet. After all of the review comments on content/05.treat.md have been addressed, I'll help create a plan to update the citation tags, merge those changes, and port your interpretation changes to a new PR. In general, it is usually cleaner to make PRs from a non-master branch in your fork, but we can work with this.

To continue editing content/05.treat.md to make the requested changes in this PR, you can go to that file in the https://github.com/greenelab/deep-review/pull/988/files tab, click the ... button in the upper right, and Edit file. Or if you were editing locally, you can continue to add commits to delton137:master and push them to delton137:master. For the suggested edits I made, you can go to the https://github.com/greenelab/deep-review/pull/988/files tab, click Add suggestion to batch, and then commit the batch of changes.

@agitter
Copy link
Collaborator

agitter commented Aug 8, 2020

@delton137 we're getting close to having the drug discovery content ready to merge. Would you be able to make edits in response to the review comments above?

@delton137
Copy link
Contributor Author

@delton137 we're getting close to having the drug discovery content ready to merge. Would you be able to make edits in response to the review comments above?

Hi, yes. Sorry for the delay. Should be able to do it today. Thanks!

Converting to Markdown format
@agitter
Copy link
Collaborator

agitter commented Aug 8, 2020

I converted the citation tags from the tsv to the Markdown link format in fea5c22 and 9a1f92a on delton137:master

@AppVeyorBot
Copy link

AppVeyor build 1.0.87 for commit fea5c22 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at:

@AppVeyorBot
Copy link

AppVeyor build 1.0.88 for commit 9a1f92a by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at:

commit agitter's changes

Co-authored-by: Anthony Gitter <[email protected]>
@AppVeyorBot
Copy link

AppVeyor build 1.0.89 for commit 617ff70 by @delton137 is now complete. The rendered manuscript from this build is temporarily available for download at:


New references: 
[@doi:10.1038/s41587-020-0418-2]
[@arXiv:1802.04364]
[@arXiv:1705.10843]
[@arXiv:1806.02473]
[@10.1021/acsmedchemlett.0c00088]
[@doi:10.1021/acs.jcim.0c00174]

note on references - I could not get DOIs for these so had to go with arXiv: 
[@arXiv:1802.04364] is published in ICML , see https://dblp.org/rec/bibtex/conf/icml/JinBJ18  
[@arXiv:1806.02473] is published in NeurIPS, see https://dblp.uni-trier.de/rec/bibtex/conf/nips/YouLYPL18 

Note: another work which led to a synthesized and tested drug molecule is this, from 2018:  https://doi.org/10.1021/acs.molpharmaceut.8b00839. 
However, the 2019 work ( Zhavoronkov et al) we discuss got much more attention. The review is already getting a bit "in the weeds"  so I left it out. 

this is a recent review (july this year) which people may be interested in. I cited it. 
https://pubs.acs.org/doi/pdf/10.1021/acsmedchemlett.0c00088

sorry for any typos that crept in - spellchecker isn't working in Github for some reason.
@delton137
Copy link
Contributor Author

@agitter I made changes to 05.treat.md, including adding a bunch of references. Please check my references. I haven't added them to content/citation-tags.tsv / content/manual-references.json because I wasn't sure if you wanted me to do that.

New references:
[@doi:10.1038/s41587-020-0418-2]
[@arXiv:1802.04364]
[@arXiv:1705.10843]
[@arXiv:1806.02473]
[@10.1021/acsmedchemlett.0c00088]
[@doi:10.1021/acs.jcim.0c00174]

Note on references - I could not get DOIs for these so had to go with arXiv:
[@arXiv:1802.04364] is published in ICML , see https://dblp.org/rec/bibtex/conf/icml/JinBJ18
[@arXiv:1806.02473] is published in NeurIPS, see https://dblp.uni-trier.de/rec/bibtex/conf/nips/YouLYPL18

Note: another work which led to a synthesized and tested drug molecule is this, from 2018: https://doi.org/10.1021/acs.molpharmaceut.8b00839.
However, the 2019 work ( Zhavoronkov et al) we discuss got much more attention. The review is already getting a bit "in the weeds" so I left it out.

this is a recent review (July this year) which people may be interested in. I cited it.
https://pubs.acs.org/doi/pdf/10.1021/acsmedchemlett.0c00088

I tried to proofread but I'm sorry for any typos that crept in - spellchecker isn't working in Github for some reason.

@AppVeyorBot
Copy link

AppVeyor build 1.0.90 for commit 8c431ee by @delton137 is now complete. The rendered manuscript from this build is temporarily available for download at:

agitter pushed a commit that referenced this pull request Aug 9, 2020
Derived from #988
and split into a new branch
@AppVeyorBot
Copy link

AppVeyor build 1.0.91 for commit d60d129 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at:

Some tags missed during conversion to Markdown link format
@agitter
Copy link
Collaborator

agitter commented Aug 9, 2020

Thanks for the updates @delton137. I'll review those soon. I pushed some updates to the citations (19e9c80) because I missed some of the tags when porting them to the Markdown link format.

I'm working on preparing a new pull request for your interpretability changes so we can move them out of the scope of this pull request.

I checked out the version of content/06.discussion.md from the delton137:master branch into a new local branch called delton137-interpret and committed it with you as the author (69ecefe). I pushed that branch to origin (https://github.com/greenelab/deep-review/tree/delton137-interpret).

@delton137 do you have this repository checked out in a local git client? If so, and assuming you have remotes origin that points to greenelab/deep-review and delton137 that points to delton137/deep-review, so you should be able to create a new pull request by doing something like:

git fetch origin
git checkout delton137-interpret
git push delton137 delton137-interpret

Then on GitHub you can create a new pull request for the delton137-interpret changes, and we can remove the changes to that file from this branch. I could start the new pull request, but then you would not be able to edit the branch during review.

Finally, in d60d129 I synced content/06.discussion.md with greenelab/master to move those changes out of this pull request.

@AppVeyorBot
Copy link

AppVeyor build 1.0.92 for commit 19e9c80 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at:

Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @delton137. Your last round of changes addressed all of my previous comments. These updates are all minor copy editing changes to improve stylistic consistency with other parts of the text. You can review and commit them if you agree.

@stephenra does this look good to you? If it does, I'm ready to merge.

content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
content/05.treat.md Outdated Show resolved Hide resolved
@stephenra
Copy link
Collaborator

@agitter Looks good to me. Thanks

agitter's copyedits

Co-authored-by: Anthony Gitter <[email protected]>
@AppVeyorBot
Copy link

AppVeyor build 1.0.93 for commit 7975507 by @delton137 is now complete. The rendered manuscript from this build is temporarily available for download at:

@delton137
Copy link
Contributor Author

delton137 commented Aug 9, 2020

@agitter I committed your changes, thanks.

Regarding the content/06.discussion.md edits.. I do have a local repository and I was able to use git fetch and checkout the delton137-interpret branch. I'll see if I can do the editing locally.

@agitter agitter merged commit 1173b9b into greenelab:master Aug 9, 2020
@agitter
Copy link
Collaborator

agitter commented Aug 9, 2020

Merged it! Let me know if you get stuck with the new pull request for the delton137-interpret branch, and we can figure out something else.

github-actions bot pushed a commit that referenced this pull request Aug 9, 2020
…esign section and discussion sections (issues with previous PR fixed) (#988)

[ci skip]

This build is based on
1173b9b.

This commit was created by the following CI build and job:
https://github.com/greenelab/deep-review/commit/1173b9b646a7a86ef64d14d1eda3bd6441c85910/checks
https://github.com/greenelab/deep-review/runs/201614289
github-actions bot pushed a commit that referenced this pull request Aug 9, 2020
…esign section and discussion sections (issues with previous PR fixed) (#988)

[ci skip]

This build is based on
1173b9b.

This commit was created by the following CI build and job:
https://github.com/greenelab/deep-review/commit/1173b9b646a7a86ef64d14d1eda3bd6441c85910/checks
https://github.com/greenelab/deep-review/runs/201614289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants