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

Central examples #756

Merged
merged 16 commits into from
Jun 29, 2023
Merged

Central examples #756

merged 16 commits into from
Jun 29, 2023

Conversation

Yoshanuikabundi
Copy link
Collaborator

@Yoshanuikabundi Yoshanuikabundi commented Jun 28, 2023

This PR includes example refreshments in anticipation of the new examples page (openforcefield/openff-docs#7)

  • conformer_energies/conformer_energies.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • experimental/openmm-import/protein-ligand.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • experimental/solvate/solvate.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • experimental/openmmforcefields.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • lammps/lammps.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • ligand_in_water/ligand_in_water.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • openmm/openmm.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • packed_box/packed_box.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • protein_ligand/protein_ligand.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category
  • vectorized_representations/vectorized_representations.ipynb
    • Check/fix title
    • Remove installation instructions (if present)
    • Ensure there's prose
    • Thumbnail
    • Category

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Yoshanuikabundi
Copy link
Collaborator Author

OK think I've done everything except thumbnails. I don't think it's a goal to have a thumbnail for every notebook, but if you have any ideas that would be cool!

It's fine for experimental/solvate to be missing a category - that's what the uncategorized category is for!

How would you like your experimental notebooks to be handled? No other project has them so you can set the standard! I see four obvious options:

  1. Treat them as normal
  2. Put them in their own category
  3. Add a warning to them in the rendered page
  4. Hide them on the examples page

But we can do something else entirely if you prefer!

@Yoshanuikabundi
Copy link
Collaborator Author

I can also do things like continue to render them, but don't show them in the gallery - so you could still link to them in your docs.

@mattwthompson
Copy link
Member

At the moment I vote

  1. Add a warning to them in the rendered page

Is this ready for review, and is there anything in particular you want me to keep an eye out for?

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

If I recall from the meeting, you consider this done and I'm responsible for taking it over the finish line? I noted two things in addition to some in-line coments

  • examples/experimental/solvate/solvate.ipynb is a little thin on guiding the user, don't need to fix it now but that one stuck out to me as needing some love.

  • I noticed some inconsistencies in the titles and prose, forgive me for not knowing the English grammar term. I'll try to illustrate it by comparing two titles:

Compute conformer energies for a small molecule with Interchange

Loading a system from OpenMM into Interchange

And picking two different bits of prose:

First we'll create an OpenMM system and topology to experiment on:

This example generates conformers for a small molecule and then evaluates their energies in OpenMM and GROMACS.

Apply the force field to the topology to generate an Interchange. This step might be slow as it needs to compute partial charges:

Then, load it into Interchange. Note that the topology mentioned here is an OpenMM topology, not an OpenFF one.

I'm trying to highlight the inconsistencies between "we'll do X," "go do X," "this example does X" (but neither your nor I are particularly involved in the process). I don't really care which is used but ideally the same is used throughout - if this is tedious and you're running low on time, just tell me which one you want used throughout the organization's documentation and I'm happy to take care of it.

"metadata": {},
"metadata": {
"tags": []
},
"outputs": [],
"source": [
"SMILES = 10 * \"C\" # \"c1n(CCO)c(C(F)(F)(F))cc1CNCCl\""
Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine to just use the interesting-looking molecule - I bet using decane was just carry-over from some debugging

examples/conformer_energies/conformer_energies.ipynb Outdated Show resolved Hide resolved
examples/conformer_energies/conformer_energies.ipynb Outdated Show resolved Hide resolved
examples/conformer_energies/conformer_energies.ipynb Outdated Show resolved Hide resolved
Comment on lines -118 to +106
"nglview.show_file(\"system.pdb\")"
"nglview.show_structure_file(\"system.pdb\")"
Copy link
Member

Choose a reason for hiding this comment

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

🧙

Comment on lines -10 to 18
"# Vectorized representations"
"Interchange can produce representations of both force field parameters and parametrized systems as NumPy arrays, which might be convenient for any number of computational approaches to force field development. This example explores this feature."
]
},
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's bizarre, they're not even in the same cell...

GitHub has rich Notebook diffs in a feature preview btw: https://github.blog/changelog/2023-03-01-feature-preview-rich-jupyter-notebook-diffs/

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #756 (07e789e) into main (f431a48) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@Yoshanuikabundi
Copy link
Collaborator Author

If I recall from the meeting, you consider this done and I'm responsible for taking it over the finish line?

Yeah I consider it "good enough" to present in the examples page. If you can add whatever changes you want (like thumbnails) and merge it that'd be great, but please don't delete the branch until it's in a release so I can continue to load this branch in the PR.

examples/experimental/solvate/solvate.ipynb is a little thin on guiding the user, don't need to fix it now but that one stuck out to me as needing some love.

Yeah I agree. I think it could include packmol as a method for instance. One of the reasons I kept it sparse (apart from just wanting to get it done) is that it feels much more on the "cookbook" side of the spectrum, as opposed to "tutorial" - so a lot of prose might be wasted at best or overwhelming at worst. Prose also has a habit of going out of date - it doesn't always get updated when the code gets fixed.

I noticed some inconsistencies in the titles and prose

Ugh you're so right.

I don't have a strong feeling either way (obviously based on the titles I've written :P). "Compute conformer energies for a small molecule with Interchange" is in what I think is called "imperative mood", which is the same as the summary sentence of a Python docstring is supposed to be, so I guess we can go with that? That way the rule is "write titles like you'd write the first sentence of a docstring". It's called imperative because it's how you'd phrase a command, which is imperative to do, but it's also how you'd end the sentence "This procedure will <title>".

In our style guide, we require Title Case for Titles, but apparently I hate title case even more than US English because I always just write titles as sentences, so that's another inconsistency you could fix if you wanted.

I'm trying to highlight the inconsistencies between "we'll do X," "go do X," "this example does X" (but neither your nor I are particularly involved in the process).

I think I'd almost always want to prioritize making the text natural and readable over consistency. Consistency in, say, a list of notebook titles definitely improves readability, but I don't think we need it everywhere.
I don't even particularly mind changing style within a single example - Using "this example" in the summary bit before the code and then assuming a conversational tone for the rest of the notebook seems fine to me, for example. Trying to keep this consistent everywhere feels like it would be very hard.

I definitely don't want to go down the passive voice route ("the topology is passed to the function as an argument") for all the usual reasons scientific writing is abandoning that tradition (not that you've suggested it!). Though sometimes that's the best way to write a particular sentence, and that's fine.

I also think it's valuable to be able to adopt different tones on the tutorial-cookbook spectrum - in a cookbook, I want the prose to be brief and then get out of the way so I can read the code, so telling the user what to do makes sense. Whereas if we're in paragraph territory an easier/more casual style is probably nicer. And then if we're just describing what happens in a cell in a single sentence between two code cells, imperative mood starts to make sense again so we can avoid a million copies of "Next, we'll <...>".

I like "we" because I like to feel like I'm working through the example with the reader as I write it, and because I find it natural to both read and write. I think sometimes I wander too far out of conversational and into, like, my every day local vernacular, and that might be an accessibility issue for non-fluent readers and Americans who don't say "keen" (:P). But usually when I mention this as self-critique people seem to like it (Australia is just so exotic). (also I parentheticalize too much even after I delete all my parentheticals)

Wow sorry stream of consciousness.

I agree that the titles should be a consistent tense, and one day it'd be nice to have 1-paragraph summaries for every notebook immediately following the title in a consistent tone, but not sure it's worth it for the rest of the prose.

If we did want to adopt more consistency, I'd suggest:

  • Titles in imperative mood ("Compute conformer energies")
  • 1-paragraph summaries immediately following titles describe the example using it as the subject in present tense ("This example computes...", "This example prepares...", not "This example demonstrates how to compute...")
  • Future tense indicative in a conversational tone for paragraphs - for example, describing goals and actions the reader and author share ("We're about to...", "We want..."). This is the "us two" or "you and I" (inclusive) version of "we", not a stand-in for "OpenFF". The vibe is that the author is talking to the reader as though they were in a classroom together, describing what they're both about to do and why. Keeping it conversational reinforces the inclusive reading of "we".
  • Don't use passive voice; if in doubt, describe the software as a subject ("Interchange produces input files", not "Input files are produced")
  • Imperative mood ending in a colon for describing the thing the code's about to do between cells ("Apply the force field to the topology to generate an Interchange. This step might be slow as it needs to compute partial charges:") (This would be a change from a lot of the notebooks I've written, where I'd tend to phrase this sort of thing as "Next, we'll apply the force field...", but this can be annoying when you have to come up with 10 different words for "next")

@mattwthompson mattwthompson merged commit c7d5ece into main Jun 29, 2023
15 checks passed
@mattwthompson
Copy link
Member

I went through the diff last night and was happy with the changes overall, and that was probably the first time in ages I've seen this all

You've convinced me that consistency across notebooks in a repo or all repos is not essential. When viewing things from the perspective of all notebooks in a single landing page, it makes some sense, I'd say. But in terms of how they're actually digested (one notebook at a time, if that) I see that's not crucial.

@mattwthompson mattwthompson deleted the central-examples branch April 16, 2024 16:16
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.

None yet

2 participants