Skip to content

Conversation

@jthorton
Copy link
Contributor

@jthorton jthorton commented Sep 5, 2025

Fixes #223

@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

github-actions bot commented Sep 5, 2025

Binder 👈 Launch a binder notebook on branch OpenFreeEnergy/ExampleNotebooks/traj-view

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

@IAlibay IAlibay Sep 23, 2025

Choose a reason for hiding this comment

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

We should make it clear here what Protocols this applies to, i.e. this is for HybridTop mostly (i.e. the tempfactor stuff is hybridtop only) but also applies for the most part to ABFEs, AHFEs, and SepTop.


Reply via ReviewNB

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

@IAlibay IAlibay Sep 23, 2025

Choose a reason for hiding this comment

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

Try adding the -nc or --no-clobber argument to wget? That way you won't double download the file.

Also this is a big nc file but the hybrid system doesn't include water. Would it be worth pulling down one of the smaller NC files? (I think we have newer generated ones?)


Reply via ReviewNB

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

@IAlibay IAlibay Sep 23, 2025

Choose a reason for hiding this comment

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

I'm not sure that the image will show up in the final html render. Can we embed the image somehow? I know that was done in the MDAnalysis UserGuide.


Reply via ReviewNB

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

@IAlibay IAlibay Sep 23, 2025

Choose a reason for hiding this comment

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

Line #3.    state_atoms = np.array([atom.ix for atom in u_0.atoms if atom.bfactor in (bfactor, 0.5, 0.0)])

Couple of things:

  1. bfactor is deprecated, use tempfactor
  2. you can simplify this and the next line (and probably speed things up) by using fancy indexing: ag = sum([u.atoms[u.atoms.tempfactors == i] for i in (0, 0.25, 0.5)])

Reply via ReviewNB

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

@IAlibay IAlibay Sep 23, 2025

Choose a reason for hiding this comment

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

I'm thinking, in MDAnalysis, it might be easy enough to align the trajectory to the coordinates of the PDB file and write that out. That's technically "centering" for free.

Also I'd be happy with merging this as-is, but opening a separate issue to try to make an MDAnalysis-only solution work. I'm sure it's doable, just needs the right set of transformations.


Reply via ReviewNB

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks for doing this!

Added a few comments for things that we should address.

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.

OpenFE Analylsis notebook

4 participants