Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Port topology notebook to PDB cookbook #1732

Merged
merged 9 commits into from
Dec 8, 2023
Merged

Port topology notebook to PDB cookbook #1732

merged 9 commits into from
Dec 8, 2023

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Oct 3, 2023

Resolves #1692

  • Figure out how JavaScript is used to display widgets on webpage
  • Tag issue being addressed
  • Add tests
  • Update docstrings/documentation, if applicable
  • Lint codebase
  • Update changelog

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #1732 (1d4711a) into main (779565b) will decrease coverage by 0.02%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

❗ Current head 1d4711a differs from pull request most recent head afb93c4. Consider uploading reports for the commit afb93c4 to get more accurate results

Additional details and impacted files

@j-wags
Copy link
Member

j-wags commented Oct 5, 2023

One of the big points about this notebook is that these PDB files are actually taken from and AMBER and GROMACS tutorials. There were a few tweaks that I had to made to make them loadable, and the PDB files have somewhat generous solvent boxes that are so large they'll gum up our CI/docs rendering. So on my to-do list is:

  • Record the code to generate these PDBs (ideally in the notebook iteself) since that will be really useful for real-world interoperability
  • Cut down the box size so we don't strain docs rendering.

@j-wags
Copy link
Member

j-wags commented Oct 12, 2023

Turning this over to @Yoshanuikabundi - Two things that would be good are:

  • Give the prose a read-over and make any edits if it's unclear
  • Checking whether the size of the solvent boxes is causing trouble in CI (either runtime or rendered docs size) and shrinking if needed.

As a stretch goal, If we REALLY wanted this to showcase interoperability, we could include the commands from the GROMACS and AMBER tutorials that made these files, but there were a few messy steps (like deleting a H from an N terminus and using RDKit to add CONECT for a ligand) that were a bit messy. So this may be more confusing than it's worth.

@j-wags j-wags requested review from Yoshanuikabundi and removed request for Yoshanuikabundi October 12, 2023 00:06
@j-wags j-wags assigned j-wags and Yoshanuikabundi and unassigned j-wags Oct 12, 2023
@mattwthompson
Copy link
Member Author

I'm personally -1 on fully documenting the process by which the data could be re-generated here. I don't think that's what we want to show off; in other words, if something changes with the tutorials I don't see a reason we should update this cookbook.

@Yoshanuikabundi
Copy link
Collaborator

@mattwthompson - for future reference, to get the widgets to display in Sphinx you just need to save the widget state in the notebook 😄 In modern versions of Notebook and Lab this is a tick box in Settings -> Save Widget State Automatically, and I think it might even notice that widget state was saved previously and tick this automatically. In older versions of notebook it was under the Widgets menu IIRC and you had to click it every time you saved the notebook.

This basically just writes the PDB file being send to NGLView in the notebook, and there's JS automatically embedded by Sphinx that loads it and runs the widgets. The catch is that there's no Python runtime in the browser, so only the stuff handled by JS is dynamic - for example, our NGLView trajectory handler passes one frame at a time from Python to NGLView, so the scan bar and play button show up but the frame never changes.

@mattwthompson
Copy link
Member Author

@Yoshanuikabundi could I convince you to capture that somewhere like the developer docs? This is valuable (and not-very-transient) information

@Yoshanuikabundi Yoshanuikabundi mentioned this pull request Dec 7, 2023
3 tasks
@Yoshanuikabundi
Copy link
Collaborator

Hey @mattwthompson, is there more you wanted to do on this? I'm going over it now but just wanted to make sure I didn't merge it without missing something you wanted.

Also

a cubic solvent box is used instead of an octohedron

Does the Toolkit not support octohedra? I thought we could handle arbitrary triclinic boxes. I can rephrase to still state it is cubic without implying that we don't support octohedra :)

Copy link
Collaborator

@Yoshanuikabundi Yoshanuikabundi left a comment

Choose a reason for hiding this comment

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

I think this is fantastic. Everything's well written and clear, it's no longer or more complex than it needs to be, and it fills an important gap in the documentation.

In my commit I've just polished things up to work a bit more like the other cookbook and clarified a few points.

The notebook adds about one minute to CI, but since notebooks only run on one of the jobs for each of Linux and MacOS I think this is acceptable. It's about 20% of the time on the Linux runner, but the MacOS runner is slower overall and it's much less significant there. I've asked PyTest to tell us which are the slowest steps of the notebook, but I'm inclined to leave it as is regardless of what it says - I think demonstrating loading a full PDB with a realistic amount of water has value.

@j-wags j-wags marked this pull request as ready for review December 7, 2023 18:31
@j-wags
Copy link
Member

j-wags commented Dec 8, 2023

I don't think that all parts of our codebase support truncated octahedrons. Interchange has some testing for exporting different sorts of boxes to GROMACS, but I don't recall adding any particular support in the Toolkit. Particularly, I don't think we support the vanilla "load PDB --> assign params --> simulate in OpenMM" workflow for octahedrons.

I'll revert that bit and merge. Thanks all!

@j-wags j-wags merged commit 1a7c558 into main Dec 8, 2023
@j-wags j-wags deleted the pdb-cookbook branch December 8, 2023 00:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document Topology.from_pdb in standalone showcase/cookbook/example
3 participants