Showcase proofreading

[atravitz]

Minor changes to address things that I noticed while running the showcase notebook.

I also cleared the notebook output to make the diffs easier to track in the future. Please let me know if this is not best-practice.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[github-actions]

👈 Launch a binder notebook on branch OpenFreeEnergy/ExampleNotebooks/showcase_proofreading

[mikemhenry]

I do think that it is best practice to not keep the output BUT we need the output so that our docs render the output:
https://docs.openfree.energy/en/stable/tutorials/rbfe_python_tutorial.html

I think what we should do is maybe have CI do the rendering (it already does the running (at least on these notebooks https://github.com/OpenFreeEnergy/ExampleNotebooks/blob/main/.github/workflows/CI.yml#L68 we will want to make sure those are a 1-1 match of the notebooks we have in our docs))
We could have the result of running the notebooks committed on merge into main? That way it won't clutter up the PR (with each CI run you would need to pull changes) BUT I don't think that would actually help since the output would still be saved...

We could have RTD keep the output which would fix the issue of the docs not having the output BUT that means that if you clone the repo or view the repo with a web browser there still would be no output.

I am not really sure what we should do here, maybe see what OpenFF does? They have the same-ish setup for embedding notebooks in their docs.

This will probably be a bigger issue to work out BUT I think the typo fixes you have here are great 🎉 so could you include the output for now? I will turn this comment into an issue that we can use to discuss what we want to do RE: notebook output being committed.

[IAlibay]

I'm going to block over outputs - needs further discussion.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

atravitz commented on 2024-10-09T21:48:09Z
----------------------------------------------------------------

The ordering of mst_network.edges seems to not be deterministic. Is that a concern, or is comparing any two edges fine for this example?

IAlibay commented on 2024-10-10T10:59:43Z
----------------------------------------------------------------

In this case it's fine, although you are right - this is a long standing issue that our network generations aren't deterministic.

[IAlibay]

In this case it's fine, although you are right - this is a long standing issue that our network generations aren't deterministic.

---

View entire conversation on ReviewNB

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

IAlibay commented on 2024-10-10T11:03:02Z
----------------------------------------------------------------

Line #5.    # Extract the content of the sdf file and visualise it

The SDF has a finite set of ligands within it, would it not be contents over content? (i.e. you have multiple entities not a single one)

atravitz commented on 2024-10-15T20:35:31Z
----------------------------------------------------------------

I think you're right, I was thinking of it in the "uncountable" way. https://dictionary.cambridge.org/grammar/british-grammar/content-or-contents. Fixed.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

IAlibay commented on 2024-10-10T11:03:02Z
----------------------------------------------------------------

I think it should be calculations (plural)?

atravitz commented on 2024-10-15T20:37:01Z
----------------------------------------------------------------

done

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

IAlibay commented on 2024-10-10T11:03:03Z
----------------------------------------------------------------

Looks like the SmallMoleculeComponent link is broken in the reviewnb preview - please double check that it still works!

atravitz commented on 2024-10-15T20:38:58Z
----------------------------------------------------------------

It was broken before this PR, but it's fixed in this version. I think it's a reviewnb bug that the entire link isn't highlighted here.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

IAlibay commented on 2024-10-10T11:03:04Z
----------------------------------------------------------------

So in other places we do enforce capital Protocol for "the class type", I'm not 100% sure if this should be capital or lowercase here - can go either way but we should be consistent.

atravitz commented on 2024-10-15T20:41:47Z
----------------------------------------------------------------

I agree that it should be capitalized when in reference to the class type. I thought this was using "protocol" in the general sense. I think we can actually just rephrase like this:

As this involves both a relative binding free energy in solvent and complex phases, RelativeHybridTopologyProtocol will be used to build two separate ProtocolDAG (directed-acyclic-graph) classes, one for each phase.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

IAlibay commented on 2024-10-10T11:03:05Z
----------------------------------------------------------------

Thank you, I appreciate folks enforcing "force field".

[IAlibay]

I'll approve, but please do look at my reviewnb comments.

[atravitz]

I think you're right, I was thinking of it in the "uncountable" way. https://dictionary.cambridge.org/grammar/british-grammar/content-or-contents. Fixed.

---

View entire conversation on ReviewNB

[atravitz]

done

---

View entire conversation on ReviewNB

[atravitz]

It was broken before this PR, but it's fixed in this version. I think it's a reviewnb bug that the entire link isn't highlighted here.

---

View entire conversation on ReviewNB

[atravitz]

I agree that it should be capitalized when in reference to the class type. I thought this was using "protocol" in the general sense. I think we can actually just rephrase like this:

As this involves both a relative binding free energy in solvent and complex phases, RelativeHybridTopologyProtocol will be used to build two separate ProtocolDAG (directed-acyclic-graph) classes, one for each phase.

---

View entire conversation on ReviewNB

[atravitz]

@IAlibay comments addressed, anything else before merging?

[IAlibay]

lgtm thanks!