Skip to content

Conversation

@hannahbaumann
Copy link
Contributor

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch OpenFreeEnergy/ExampleNotebooks/septop_analysis

@hannahbaumann hannahbaumann linked an issue Aug 27, 2025 that may be closed by this pull request
@@ -0,0 +1,124 @@
{
Copy link
Member

@IAlibay IAlibay Aug 27, 2025

Choose a reason for hiding this comment

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

Ideally some pre-amble text / title would be great.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this!

@@ -0,0 +1,124 @@
{
Copy link
Member

@IAlibay IAlibay Aug 27, 2025

Choose a reason for hiding this comment

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

Line #15.                dg_0 = json_0["estimate"].magnitude

Would need to guard this with some try/excepts here, in case:

  • You picked up a file that isn't a results JSON.
  • The estimate is None for any of the repeats.
  • There is not file for some repeats.

Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is now filtering for result json files, raises a ValueError if the estimate is None, and when only a single repeat is provided, uses the MBAR error as uncertainty and uses the standard deviation if results from more results are provided. We may want to change this in the future.

@@ -0,0 +1,124 @@
{
Copy link
Member

@IAlibay IAlibay Aug 27, 2025

Choose a reason for hiding this comment

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

We'd probably want to give them a few flavours of this:

  • What if they want to look at the individual legs of the cycle (i.e. look if things are going wrong with the solvent or complex leg)?
  • What if they want to get a dG (here we can just refer to the cinnabar cookbook).

Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these!

@hannahbaumann hannahbaumann requested a review from IAlibay August 28, 2025 11:26
@@ -0,0 +1,1065 @@
{
Copy link
Contributor

@jthorton jthorton Aug 28, 2025

Choose a reason for hiding this comment

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

Line #12.            # Use standard deviation as error when more than 1 repeat

Maybe this is a more general problem with the error estimation but you would want to switch all edges to use the MBAR error if any edge has a single repeat; otherwise, the errors might have very different magnitudes due to how the MBAR estimator works and you would bias the MLE predictions to the edges with errors from MBAR.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jthorton , I agree, that would be better! I won't have time to do this before my vacation, would you maybe be able to change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem I can add that in!

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in 88bce0a

Copy link
Contributor

@jthorton jthorton left a comment

Choose a reason for hiding this comment

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

LGTM!

@IAlibay IAlibay merged commit 5eb2a12 into main Sep 1, 2025
4 checks passed
@IAlibay IAlibay deleted the septop_analysis branch September 1, 2025 13:27
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.

Septop Network Analysis notebook

4 participants