-
Notifications
You must be signed in to change notification settings - Fork 20
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
Edits to User Guide documentation #995
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #995 +/- ##
==========================================
- Coverage 94.59% 92.84% -1.76%
==========================================
Files 134 134
Lines 9940 9940
==========================================
- Hits 9403 9229 -174
- Misses 537 711 +174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I'm going to put a block here because it looks like some of the things here will need some discussion / will have differing views.
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
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.
Much clearer and concise, great work! I just added some comments.
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.
Thank for you all your work on this @atravitz , this looks great overall! Just left a few suggestions!
Co-authored-by: Iván Pulido <[email protected]> Co-authored-by: Hannah Baumann <[email protected]> Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Iván Pulido <[email protected]>
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.
@IAlibay A couple items I addressed but you should take one more look at. I think we need to revisit the ligand network section as a whole. Maybe a follow-up PR?
The :func:`default_lomap_score` function combines several criteria | ||
(such as the number of heavy atoms, if certain chemical changes are present, | ||
and if ring sizes are being mutated), into a single value. | ||
It is possible to combine scoring functions in this way because each scoring function returns a normalized value. |
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.
let me know if this is better.
Also while we are at it, I think we should have a call to action on this page: In between the Also also, feel free to just convert this comment into an issue if you think it warrants further discussion and/or is outside the scope of the PR. |
Co-authored-by: Mike Henry <[email protected]>
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, lgtm!
Edits just to the top level and Setup section. Further edits for other sections (and to update graphics) will follow in later PRs.
Developers certificate of origin