-
Notifications
You must be signed in to change notification settings - Fork 664
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
Improved, Best-like contact analysis (Issue #702) #708
Conversation
A worked example with GpA dimer in a bilayer Added tests to the BestHummerContacts calculation Tests for fraction of native contacts (Q)
@@ -146,14 +144,15 @@ | |||
import errno | |||
import warnings | |||
import bz2 | |||
from six.moves import zip | |||
from itertools import izip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should keep from six.moves import zip
zip
here is izip
in py2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the six import was correct. It does import the iterator version of zip under python2 as zip. So no more izip. This is part of our effort to support python3 along with python2 in the future.
- removed a dependancy on pandas, update tests
Ah, crap - it seems that I applied my changes to some ghetto-old version of contacts.py. This is now changed: the only part of contacts.py that differs from the version in So basically all your comments about contacts.py are valid but to some old version of contacts.py The only thing that I really added is:
|
step : int, optional | ||
Step between frames to analyse, Default: 1 | ||
""" | ||
self.u = u |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to pull the reference to universe off grA to reduce the number of arguments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, provided that
assert(grA.universe == grB.universe)
I prefer passing groups instead of atom selection string. Whenever i pass a selection string to rms_fit_trj
and i cock something up (selections are not identical, etc) i takes an extra step to debug it (gotta create the AtomGroups by hand). Plus people may be constructing AtomGroups by hand in this scenario.
@richardjgowers many thanks for all your comments and helping me navigate in the conventions! The tests fail because I use a function that depends on scipy - that used to be a dep of MDAnalysis, what's the replacement now?
|
results: list | ||
Fraction of native contacts for each frame | ||
""" | ||
assert(grA.universe == grB.universe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want an assert here, do a proper if not x==y: raise ValueError with a nice message
I think scipy is optional now, not sure there's a workaround for sparse matrices. You can decorate the test in a skipif (grep for another scipy test in analysis), we run 2 lots of tests, minimal and full. |
Build passes, allelujah! |
backlink to #702 |
Thanks for giving me some time |
@kain88-de @orbeckst all done here again (I think...) added numpy style docs, reworked the test, fixed the staticmethod problem |
- ContactAnalysis was removed by @jandom but needs to stay because the functionality does not exist in the new Contact class (But it needs to be rewritten with AnalysisBase.) - updated doc strings (numpy style)
…o issue-702-contacts
|
||
logger = logging.getLogger("MDAnalysis.analysis.contacts") | ||
|
||
|
||
class ContactAnalysis(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put ContactAnalysis
back in jandom#1 – it should not have been removed. Raise an issue to overhaul ContactAnalysis
to work within the new analysis model. We can then also rename it and break it's API, or for all I care, completely rewrite with more focused functionality. But for right now it has to stay.
@jandom , add an entry to CHANGELOG! |
Aye, no problem adding back |
@jandom, did you merge my PR? |
self.outfile = outfile | ||
|
||
@staticmethod | ||
def load(filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method uses self, it cannot be a static method.
…alysis into orbeckst-issue-702-contacts Conflicts: package/MDAnalysis/analysis/contacts.py
This looks done to me. I added the WIP flag so that it doesn't get merged for 0.14.x (as discussed with @kain88-de ). That was a long process but along the way we made big strides towards a better analysis module #719 . Many thanks to @jandom and everyone else involved in the discussion. |
- numpy style docs - guard scipy.sparse import and warn - see PR #708 for discussion
I merged it by hand see 4ee46b. |
@kain88-de ditto #702 :) |
If this is now on develop then the current develop will have to become 0.15.0. If we want a 0.14.1 then we need to branch from before this merge. With semantic versioning we can't have new features in a patch release. Oliver Beckstein Am Feb 29, 2016 um 1:34 schrieb kain88-de [email protected]:
|
There isn't much for the patch release. We should be able to cherry-pick them into a |
I've started working on improvements of the new I want to add some additional examples and documentation to the module. For this it would be nice to know what other people want to use the contact analysis for. Use Cases
It would be nice if others contribute as well what this type of analysis can be used for. That way I can write examples and add tests for all of them (hopefully). History
|
Actually, let's have this discussion in a new issue! |
- numpy style docs - guard scipy.sparse import and warn - see PR MDAnalysis#708 for discussion
Addresses #702.
A worked example with GpA dimer in a bilayer
Added tests to the BestHummerContacts calculation
Tests for fraction of native contacts (Q)