-
Notifications
You must be signed in to change notification settings - Fork 18
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
Concurrence #28
Concurrence #28
Conversation
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.
looks good to me, some minor documentation and readability issues
contact_map/concurrence.py
Outdated
---------- | ||
values : list of list of bool | ||
the whether a contact is present for each contact pair at each | ||
point in time; outer list is length number of frames, inner list |
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 is the other way around outer list is length contact pair and inner list is frame
contact_map/concurrence.py
Outdated
labels = [str(contact[0]) for contact in atom_contacts] | ||
distances = md.compute_distances(trajectory, atom_pairs=atom_pairs) | ||
vector_f = np.vectorize(lambda d: d < cutoff) | ||
values = list(map(list, zip(*vector_f(distances)))) |
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.
please add a comment for this magic
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.
and this could be changed into: list(map(list, vector_f(distances).T)
as you do want to map the transpose of the array to a list and I suspect this is more performant as it used numpy instead of zip. But I didn't check my suspicion
contact_map/concurrence.py
Outdated
distances = md.compute_distances(trajectory, | ||
atom_pairs=atom_pairs) | ||
min_dists = [min(dists) for dists in distances] | ||
values.append(list(map(lambda d: d < cutoff, min_dists))) |
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.
list comprehension should be more readable
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.
One last question on the isinstance
check, LGTM otherwise.
contact_map/concurrence.py
Outdated
list : | ||
list in the format of ``ContactCount.most_common()`` | ||
""" | ||
if isinstance(contact_input, contact_map.ContactFrequency): |
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.
shouldn't this be isinstance(contact_input, contact_map.ContactObject)
? As this statement would be False
for a ContactMap
or ContactDifference
contact_map/concurrence.py
Outdated
""" | ||
if isinstance(contact_input, ContactObject): | ||
contact_input = contact_input.contacts[atom_or_res] | ||
if isinstance(contact_input, contact_map.ContactCount): |
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.
can you put an extra blank line, this tripped me up and I read if
, elif
When looking at the behavior of contacts, it is often not only important to see how often a contact occurs, but also what other contacts occur simultaneously. These simultaneous contacts (which we refer to as "concurrence") can be used to define stable states.