Replace ring finding code #171
Conversation
|
This is still a WIP. I need to set up some unit tests and check to see how much of a performance hit this takes. The code included now was just the first successful attempt, so I'm sure there are plenty of optimizations that can be made. @mikemhenry I tested this code locally on your system (from #170) and all atom types were assigned correctly, so you can checkout this branch if you need to atom type similar systems in the meantime before this is merged. |
|
This is great, works for me and will be REALLY useful in other compounds I'm working with! Also I really wouldn't worry about performance hits since with the residue mapping, each residue is only typed once. |
|
I still need to go through and clean up this code. As far as unit tests go, I'm thinking about setting up tests that atomtype a few different systems (e.g. a polymer, a surface, and a buckyball) and marking the tests with a timeout using pytest-timeout. This will help alert us if any changes to the code (such as this case where we're replacing the ring-finding algorithm) are significantly reducing performance. |
|
If pytest supports it, also running the test like 3 times to make sure
things stay deterministic would be good to. Also an assertion that the atom
types are correct would be good to.
…On Thu, Aug 16, 2018, 22:21 Andrew Z. Summers ***@***.***> wrote:
I still need to go through and clean up this code. As far as unit tests
go, I'm thinking about setting up tests that atomtype a few different
systems (e.g. a polymer, a surface, and a buckyball) and marking the tests
with a timeout using pytest-timeout
<https://bitbucket.org/pytest-dev/pytest-timeout/>. This will help alert
us if any changes to the code (such as this case where we're replacing the
ring-finding algorithm) are significantly reducing performance.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#171 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ALOI3iAFCtGxHpimAwroPJvxsji-l1ghks5uRkSzgaJpZM4UlNv4>
.
|
|
|
||
| _prepare_atoms(top, compute_cycles=True) | ||
| cycle_lengths = [list(map(len, atom.cycles)) for atom in top.atoms()] | ||
| expected = [5, 6, 6] |
There was a problem hiding this comment.
I might be thinking about this incorrectly but isn't this an entire fullerene structure? How can there only be 3 cycles?
There was a problem hiding this comment.
expected here is the expected cycle lengths for the cycles each atom is included within. Every atom in the fullerene should be included in two six-membered rings and one five-membered ring. In the assertion I'm looping over cycle_lengths and performing the comparison on the cycles for each atom.
|
Performance tests have been added. I'm not sure we actually need additional tests for correct atom-typing and deterministic behavior since we already test those aspects with |
|
This looks fantastic - thanks @summeraz! |
Our previous approach to finding the rings that each atom is included within, which relied upon NetworkX's cycle_basis function, did not guarantee that all rings would be found for multi-cyclic systems. This function would return a set of rings from which the full bond graph could be recreated, but this would not necessarily be all rings in the system.
This PR replaces
cycle_basiswith a function that should find all (chordless) cycles (i.e. rings) that each atom is included within, loosely based off of this StackOverflow post.Resolves #170