Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ligand binding mode clustering with PyEMMA #13

Merged
merged 29 commits into from
Nov 11, 2017
Merged

Ligand binding mode clustering with PyEMMA #13

merged 29 commits into from
Nov 11, 2017

Conversation

nathanmlim
Copy link
Collaborator

This PR adds a notebook with example files for doing tiCA and clustering to get ligand binding modes from an MD simulation.

Addressing part of issue 5

OpenMM, MDTraj, clustering and visualization: Probably should be updated/extended to use examples from @nathanmlim 's work on BLUES

@davidlmobley
Copy link
Member

Cool, thanks! Will try it out soon. :)

Copy link
Member

@davidlmobley davidlmobley left a comment

Choose a reason for hiding this comment

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

Reviewing this, a few comments:

  • I think you said you adapted this from the MSM workshop; can you give credit? (e.g. add a mention of that, and also add authors indicating whoever's it was originally -- Frank's (?) -- plus you if you've made significant update make sure you have yourself as an author
  • There is lots of stuff here, so try and give credit where credit is due for anything you took from elsewhere and adapted
  • You have a plot of lag time; can you say something about what that's for/how it's used? It's non-obvious. (Also, the next box uses a lag time of 80 ps but it's not said why...)
  • Shortly after that, it says "Plot the first two TICA components from both models." What are the "both models"? Could be more clear.
  • To the degree that you're able to comment the code slightly better, that would be helpful, but I can also address this once merged

On the whole it looks like it should basically be good to go, just perhaps as you make the updates above, read it through one more time and keep in mind the target audience (someone who has never done clustering and will want to do something like what you've done in BLUES but for a different problem) and try and make sure there is enough detail.

Tag me once you've made these changes and we can ge it merged and then I'll do some more editing myself.

@nathanmlim
Copy link
Collaborator Author

@davidlmobley Updated!

@davidlmobley
Copy link
Member

Thanks, @nathanmlim !!

@davidlmobley davidlmobley merged commit 62a6906 into master Nov 11, 2017
@davidlmobley davidlmobley deleted the clustering branch December 27, 2017 17:55
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.

2 participants