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

Contacts #84

Merged
merged 6 commits into from
Oct 6, 2020
Merged

Contacts #84

merged 6 commits into from
Oct 6, 2020

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Jun 29, 2020

Update contacts notebooks

  • squashed Last executed and Last updated into one line
  • Removed special section for visualisation packages
  • Added "see also"
  • Changed 'selection' keyword to 'select'
  • Added radius_cut_q to this notebook and explained results more
  • Added background section to this notebook
  • Added bit more intro to RST readme

@dottiejean
Copy link

Update contacts notebooks

  • squashed Last executed and Last updated into one line
  • Removed special section for visualisation packages
  • Added "see also"
  • Changed 'selection' keyword to 'select'
  • Added radius_cut_q to this notebook and explained results more
  • Added background section to this notebook
  • Added bit more intro to RST readme

Copy link

@dottiejean dottiejean left a comment

Choose a reason for hiding this comment

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


  • ```````

@lilyminium lilyminium mentioned this pull request Jul 7, 2020
@lilyminium
Copy link
Member Author

Hi @dottiejean, welcome to MDAnalysis! Did you have thoughts on this PR? I think your comments may have been a bit mixed up :-)

@lilyminium lilyminium requested a review from orbeckst July 8, 2020 07:09
@lilyminium lilyminium marked this pull request as ready for review July 9, 2020 09:52
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Minor suggestions

contacts_within_cutoff.ipynb

http://minium.com.au/UserGuide/examples/analysis/distances_and_contacts/contacts_within_cutoff.html

  • "You may not want to use this definition for real work."

    -> "It is not recommend to use this overly simplistic definition for real work that you want to publish."

  • Perhaps show how to turn contacts_within_cutoff() into a analysis class with analysis_from_function() or similar?? But that's very optional... and might distract.

contacts_native_fraction.ipynb

http://minium.com.au/UserGuide/examples/analysis/distances_and_contacts/contacts_native_fraction.html

  • I would not use a Note box to highlight the 3 different methods as it looks as if there's something wrong or really dangerous going on even though you're just providing information. A simple subsection might do the job better. What's missing is an introduction that also explains why I should care about these three methods.

  • "For a tutorial on writing your own custom metric, please see the the tutorial on writing your own contacts analysis."

    could then be a See Also in this section.

  • "Native contacts are those contacts that exist in the fully-folded protein structure (as opposed to non-native contacts, which may be formed along the folding pathway but are not present in the final structure). "

    It's not just folding and I would say more generally when there are multiple states. In the AdK example, we have two folded states (closed and open) and we look at a transition between them. The native contacts along the transition change.

    I see that you are leading the reader to this conclusion in contacts_native_fraction.ipynb but perhaps make it less of a surprise by defining the more general case first and then later come back to it in the example.

  • Instead of using (pseudo) salt bridges, C-alpha native contacts should work better (see, for instance, Fig A https://journals.plos.org/ploscompbiol/article/figure?id=10.1371/journal.pcbi.1004568.g007 where we plotted native contacts relative to the open (4AKE) and closed (1AKE) conformation of AdK – yes, that's a q1/q2 analysis but it should also show something sensible in a single coordinate). See Methods for the definition ("We define a contact to be a residue pair whose Cα atoms are separated by a distance smaller than 8 Å,")

    Don't get me wrong, the way the tutorial is presented with pseudo-saltbridge contacts is very nicely done and does not have to be changed. I just wanted to add a bit of context, given that we had looked at this transition with native contacts in the past.

    Ok, fine... you're doing exactly the 8Å CA contact in the q1-q2 notebook... so just ignore what I wrote, but I am also not willing to delete it again ;-).

  • The hack with passing (acidic_2, basic_2) as the ref from the last frame is good but understanding it requires to know why you have u and ref and that this only works because after ref.trajectory[-1], the trajectory frame is never moved again. Perhaps make sure to say that it's important that u and ref are two separate universes and that it is ok to take AtomGroups from different universes as long as they give equivalent positions.

contacts_q1q2.ipynb

http://minium.com.au/UserGuide/examples/analysis/distances_and_contacts/contacts_q1q2.html

  • reference (#best_native_2013) in Note is not resolved
  • cite the _Joel Franklin, Patrice Koehl, Sebastian Doniach, and Marc Delarue. _ paper for Q1-Q2, that's really where it comes from

@dottiejean
Copy link

I'm not totally understand what I am doing. Lol

@orbeckst
Copy link
Member

orbeckst commented Aug 4, 2020

@lilyminium do you want to report the baby bot? They have a menu on the message and a category for meat in a can.

@orbeckst orbeckst mentioned this pull request Aug 8, 2020
7 tasks
@dottiejean
Copy link

I'm not really sure what to do here offer my 8nsights??

@lilyminium lilyminium merged commit 5332b11 into MDAnalysis:master Oct 6, 2020
@lilyminium lilyminium deleted the contacts branch October 6, 2020 02:16
lilyminium added a commit that referenced this pull request Jan 21, 2022
- Changed 'selection' keyword to 'select'
- Added radius_cut_q and explained results more
- Added background section
- Added bit more intro to readme
lilyminium added a commit that referenced this pull request Jan 21, 2022
- Changed 'selection' keyword to 'select'
- Added radius_cut_q and explained results more
- Added background section
- Added bit more intro to readme
lilyminium added a commit that referenced this pull request Jan 21, 2022
- Changed 'selection' keyword to 'select'
- Added radius_cut_q and explained results more
- Added background section
- Added bit more intro to readme
lilyminium added a commit that referenced this pull request Jan 22, 2022
- Changed 'selection' keyword to 'select'
- Added radius_cut_q and explained results more
- Added background section
- Added bit more intro to readme
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.

3 participants