Skip to content

update for new cinnabar API#311

Open
jthorton wants to merge 3 commits into
mainfrom
cinnabar-update
Open

update for new cinnabar API#311
jthorton wants to merge 3 commits into
mainfrom
cinnabar-update

Conversation

@jthorton

@jthorton jthorton commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

Colab 👈 Launch a Colab session on branch cinnabar-update

@@ -5,69 +5,29 @@
"id": "ddae61fd-fa15-4be9-9160-ba3e195c67dc",
"metadata": {},
"source": [
"# Plotting OpenFE DG results against experiment using Cinnabar v0.4\n",
"# Plotting OpenFE DG results against experiment using Cinnabar v0.6.0\n",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This notebook is not included in the current docs do we still want this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be useful for people who run ABFEs not having to go throught he RBFE related content in the general plotting notebook, but maybe it's also not super important? If we keep it, we should definitely add it to the docs!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note: Personally I'm in support of merging the notebooks.

I think this notebook was necessary because some folks were asking how they could plot their absolute results and they didn't know how. We just need to make sure that's got a proper example somewhere.

@@ -5,69 +5,29 @@
"id": "ddae61fd-fa15-4be9-9160-ba3e195c67dc",

@hannahbaumann hannahbaumann Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could also add here or gathered with gather_abfe and refer to the ABFE CLI tutorial?


Reply via ReviewNB

@@ -5,9 +5,9 @@
"id": "ddae61fd-fa15-4be9-9160-ba3e195c67dc",

@IAlibay IAlibay Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We probably need to make it clearer that this tutorial is geared solely towards relative free energies.


Reply via ReviewNB

@@ -5,9 +5,9 @@
"id": "ddae61fd-fa15-4be9-9160-ba3e195c67dc",

@IAlibay IAlibay Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"gather the experiemtnal" -> typo

"to have units" -> Can you expand and mention OpenFF units here?


Reply via ReviewNB

@@ -5,9 +5,9 @@
"id": "ddae61fd-fa15-4be9-9160-ba3e195c67dc",

@IAlibay IAlibay Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Line #8.            value=row["DDG(i->j) (kcal/mol)"] * unit.kilocalories_per_mole,  # the computed relative free energy change for the transformation

[nit] I like these inline comments, but it might be easier for folks to read them if you put the comment on the line before the kwargs rather than trying to have it all on the same line.


Reply via ReviewNB

@@ -5,9 +5,9 @@
"id": "ddae61fd-fa15-4be9-9160-ba3e195c67dc",

@IAlibay IAlibay Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dervied -> derived


Reply via ReviewNB

@IAlibay IAlibay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Couple of typos, otherwise the content changes look good.

I would advocate for merging post content fix and then opening a follow-up PR / issue to deal with the absolute cookbook (my suggestion would be to try to merge if possible, otherwise maybe breaking down the one tutorial into more cookbooks).

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