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_notebook

@hannahbaumann
Copy link
Contributor Author

hannahbaumann commented Apr 24, 2025

I'm currently creating the network using an atom mapper which may be confusing. I could instead either do a star map, looping over the ligands[1:] creating transformations using the first ligand as center, not creating a LigandNetwork first. Or load in a file with the names of the edges that we want to run (this is what I've been doing as I wanted to use the same edges in industry benchmarking). Or only show a single transformation between two random ligands?

@IAlibay
Copy link
Member

IAlibay commented Apr 24, 2025

I think doing it with network planning is fine, just have a bolded note that says that the mapping isn't used outside of the score.

Alternatively manually specifying edges is also fine IMHO.

@hannahbaumann hannahbaumann requested a review from jthorton April 24, 2025 13:53
@@ -0,0 +1,588 @@
{
Copy link
Member

@IAlibay IAlibay Apr 24, 2025

Choose a reason for hiding this comment

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

We're doing this here for SepTop but not for ABFE. Not sure if intentional but just checking.


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.

Ah yes, I had used different templates for the two notebooks. Should I add this to the ABFE notebook?

Copy link
Member

Choose a reason for hiding this comment

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

Long term I think the idea would be to replace this with a "our ligands are pre-charged, see this cookbook here for an example of how you can go about charging your ligands".

Probably best to just add this into ABFEs for now and fix both the notebooks later to match whenever we're ready to make the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

@hannahbaumann hannahbaumann requested a review from IAlibay April 29, 2025 13:34
@@ -0,0 +1,595 @@
{
Copy link
Member

@IAlibay IAlibay Apr 29, 2025

Choose a reason for hiding this comment

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

Is this is the right diagram? (looks different from Fig 1 from your paper)


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.

In the paper I'm only showing the complex leg in detail, I thought that might be confusing here, and that it would be better to just show the simple full cycle. But I can also change it to the figure from the paper!

Copy link
Member

Choose a reason for hiding this comment

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

That's fair, this works for me if it works for you.

@IAlibay IAlibay merged commit cd91b7e into main Apr 30, 2025
2 of 4 checks passed
@IAlibay IAlibay deleted the septop_notebook branch April 30, 2025 13:42
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