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

Edits to User Guide documentation #995

Merged
merged 25 commits into from
Nov 15, 2024
Merged

Edits to User Guide documentation #995

merged 25 commits into from
Nov 15, 2024

Conversation

atravitz
Copy link
Contributor

@atravitz atravitz commented Oct 31, 2024

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

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (4c06370) to head (6e44bfb).
Report is 26 commits behind head on main.

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     
Flag Coverage Δ
fast-tests 92.84% <ø> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atravitz atravitz marked this pull request as ready for review November 1, 2024 21:38
Copy link
Member

@IAlibay IAlibay left a 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.

docs/guide/results/index.rst Outdated Show resolved Hide resolved
docs/guide/setup/alchemical_network_model.rst Show resolved Hide resolved
docs/guide/setup/defining_protocols.rst Outdated Show resolved Hide resolved
docs/guide/setup/defining_protocols.rst Outdated Show resolved Hide resolved
docs/guide/setup/defining_protocols.rst Outdated Show resolved Hide resolved
docs/guide/setup/defining_protocols.rst Outdated Show resolved Hide resolved
docs/guide/setup/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ijpulidos ijpulidos left a 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.

docs/guide/introduction.rst Outdated Show resolved Hide resolved
docs/guide/introduction.rst Show resolved Hide resolved
docs/guide/introduction.rst Outdated Show resolved Hide resolved
docs/guide/introduction.rst Outdated Show resolved Hide resolved
docs/guide/introduction.rst Show resolved Hide resolved
docs/guide/setup/creating_atom_mappings_and_scores.rst Outdated Show resolved Hide resolved
docs/guide/setup/creating_ligand_networks.rst Outdated Show resolved Hide resolved
docs/guide/setup/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@hannahbaumann hannahbaumann left a 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!

Copy link
Contributor Author

@atravitz atravitz left a 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?

docs/guide/introduction.rst Outdated Show resolved Hide resolved
docs/guide/introduction.rst Outdated Show resolved Hide resolved
docs/guide/introduction.rst Show resolved Hide resolved
docs/guide/setup/index.rst Outdated Show resolved Hide resolved
docs/guide/setup/creating_atom_mappings_and_scores.rst Outdated Show resolved Hide resolved
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.
Copy link
Contributor Author

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.

docs/guide/setup/defining_protocols.rst Outdated Show resolved Hide resolved
docs/guide/setup/alchemical_network_model.rst Show resolved Hide resolved
@mikemhenry
Copy link
Contributor

+1 to removing see also from the TOC but keeping that section on the page
image

@mikemhenry
Copy link
Contributor

same with references
image

@mikemhenry
Copy link
Contributor

Also while we are at it, I think we should have a call to action on this page:
https://openfe--995.org.readthedocs.build/en/995/guide/introduction.html

In between the Introduction and Workflow overview section as a .. note:: that is something like "If you have used openfe, please use this citation xxx." I think some sentence about also sponsoring the project would be good as well.

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.

Copy link
Contributor

@hannahbaumann hannahbaumann left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm!

@atravitz atravitz merged commit 9b0b4b2 into main Nov 15, 2024
11 checks passed
@atravitz atravitz deleted the docs_user_guide_edits branch November 15, 2024 14:57
@atravitz atravitz mentioned this pull request Nov 15, 2024
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.

5 participants