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

Cookbook flowchart #520

Merged
merged 31 commits into from
Aug 29, 2023
Merged

Cookbook flowchart #520

merged 31 commits into from
Aug 29, 2023

Conversation

Yoshanuikabundi
Copy link
Contributor

@Yoshanuikabundi Yoshanuikabundi commented Aug 10, 2023

This PR organises the Cookbook into a flowchart. As the cookbook is filled out, users will be able to see and locate the cookbook entries for their task graphically.

Opening this PR for feedback!

Developers certificate of origin

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (25fb327) 91.29% compared to head (90026cc) 91.29%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #520   +/-   ##
=======================================
  Coverage   91.29%   91.29%           
=======================================
  Files         116      116           
  Lines        7056     7056           
=======================================
  Hits         6442     6442           
  Misses        614      614           
Files Changed Coverage Δ
openfe/setup/ligand_network.py 100.00% <ø> (ø)

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

@Yoshanuikabundi Yoshanuikabundi marked this pull request as ready for review August 21, 2023 10:48
@Yoshanuikabundi
Copy link
Contributor Author

OK I think this is ready for final review! If you've looked at it previously, you might need to do a shift+f5 on the RTD page to force your browser to download the updated stylesheet. Future PRs should be smaller as they can just fill in one arrow at a time, so this one helps us visualize what parts of the cookbook are missing!

Copy link
Member

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

I like a lot of this, but a few changes are probably needed.

Conceptually, I'm not sure that it is accurate to have the simplified run section for one workflow and not the other. The run section is the same no matter how you got there.

The run section with detailed run stages is potentially useful, although there won't be any cookbook materials about it here. The ideas are relevant for users, because it explains some of how the simulations are actually run (and they see that in outputs, for example). But only developers of new protocols or executors will actually interact with it directly (that stuff lives in gufe).

Problems with LigandNetwork

This is incorrect:

image

A LigandNetwork is the output of a ligand network planning function. If consists of LigandAtomMappings associated with its edges, and SmallMoleculeComponents as its nodes.

Problems with the run section

image

You probably want to put a ProtocolUnitResult between ProtocolUnit and ProtocolDAGResult, and you probably want to show that many ProtocolDAGResults feed into a single ProtocolResult at the end.

Reason number 10294 that I find straight CSS to be a nightmare....

Safari 15.5 (as rendered in the PR on RTD):

image image

@Yoshanuikabundi
Copy link
Contributor Author

Yoshanuikabundi commented Aug 23, 2023

  • Move "Transformation-first" workflow to it's own cookbook, call it "Under the hood", and refocus it for developers (ie, remove the LigandNetwork box)
  • Make "Run", "Setup", etc headings slightly larger
  • Insert ProtocolUnitResult after ProtocolUnit (single arrow)
  • Use multiple arrows combining into one arrow from ProtocolUnitResult to ProtocolDAGResult and from components into ChemicalSystem
  • Add "Gather" section to under-the-hood flowchart describing combining multiple ProtocolDAGResult into a single ProtocolResult
  • Reduce granularity of LigandNetwork creation - replace LigandAtomMapper box with Orion/FEP+ and replace "Atom Map Scorers" with manual definitions of a ligand network.
  • Fix Safari

@Yoshanuikabundi
Copy link
Contributor Author

@dwhswenson - The flowchart looks perfect on Safari 16.5.1 with no extensions, as well as on both Firefox and Chrome. This is encouraging because it suggests that I'm not doing anything wierd by all three major web engine's latest versions, so I think the current implementation will basically work. But I can definitely try and write fallback CSS for features that have only recently been added! Could you let me know what version of Safari you're using?

@richardjgowers
Copy link
Contributor

@Yoshanuikabundi I'm in 16.3 and I get the FUBAR that @dwhswenson gets too. That said I think we can merge this as-is and do Safari fixes in a future PR since this is pretty neato

@dwhswenson
Copy link
Member

Agreed that "Fix Safari" can be moved to a future PR (other points should still be addressed here). I'm on Safari 15.5; extensions deactivated (only 1Password and Keyword Search installed), viewing the RTD render of the PR. I haven't tested with local builds to see if it is possible that RTD is serving something causing this; @Yoshanuikabundi, did you test in Safari viewing a local build or using the RTD render? (@richardjgowers, I assume you were looking at the RTD render, right?)

@mikemhenry
Copy link
Contributor

Oh this is really nice! I love that you can click on it!
image

On firefox 115.0 (64-bit) (linux) there is a tiny artifact. Looks like this is the element
image

I wouldn't worry about it unless you have seen it before or something and the fix is easy.

@Yoshanuikabundi
Copy link
Contributor Author

Okie dokes! I tested on RTD on Safari. I think it's just one or two CSS features that older Safaris don't have, but I'll fix them up in another PR. It'll probably look similar on older versions of Chrome and Firefox, but I think most users of those browsers are relatively up-to-date.

@mikemhenry Yeah I noticed that while writing it but it appears correct on Chrome and doesn't appear to be particularly consistent on Firefox... So I'm chalking it up to a Firefox bug.

I'm glad you like it though! I think having flowcharts written in the same markup as everything else so you can have links and so on is very powerful, and having them pre-rendered is great - users don't need to wait the second or so it takes mermaid to render. I was pretty surprised I was able to make them look this good, which I guess just goes to show how far CSS has come! As emphasised by the fact it breaks in very recent versions of Safari...

@Yoshanuikabundi
Copy link
Contributor Author

#527 should add a link to the newly documented load network functions to the appropriate part of the flowchart. Other than that, I think this is good to go as long as the build works.

Copy link
Member

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

Looking forward to the eventual Safari fix; everything else lgtm!

@Yoshanuikabundi Yoshanuikabundi merged commit 2e50b8c into main Aug 29, 2023
7 checks passed
@Yoshanuikabundi Yoshanuikabundi deleted the cookbook-flowchart branch August 29, 2023 03:58
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.

4 participants