-
Notifications
You must be signed in to change notification settings - Fork 657
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
different results of select_atoms in 0.17.0 and 0.16.2 #1795
Comments
It would be helpful if you could provide a minimum working example for the issue -- for example a small sample input file to go along with this report. |
Yes, sure. I should be more precise. You can reproduce this problem with following code:: import MDAnalysis as mda
U = mda.Universe('atom_selection_problem.pdb','atom_selection_problem.dcd')
U.select_atoms('(resname WAT or resname HOH) and (sphzone 6.0 (resnum 99 or resnum 147 or resnum 231 or resnum 261 or resnum 289))').resnums Run this code with 0.16.2 and 0.17.0 and compare results. On my system, 0.16.2 gives 328, 411, 444, 495, 835 but 0.17.0 gives 820, 1518, 1633, 2280, 2325, 2518, 2562, 2583, 2924, 3251, 3426, 3543, 3791, 3854, 3871, 4243, 4397, 4463, 4566, 4815, 4932, 5638, 5642, 5734, 6081, 6254, 6374, 6390, 6425, 6513, 6736, 7044, 7353, 7416, 7534, 7608, 7768, 8436, 8438, 8545. So, in case of 0.17.0 it is 40 residues (not 31). Any way, it looks very strange. |
Once someone reproduces (ideally with a test file) then we need to label this as defect and fix. |
Tentatively assigning myself to take a look for a bit here. |
I've bisected over roughly two years of history to find this:
|
I thought we were going to remove those pbc flags in favor of some kid of keywords -- I'll take a look at the same location in code at most recent commit hash. |
Is the difference applying PBC by default? Are the additionally found waters across the box's periodic boundary? |
BTW, @tylerjereddy , please add the defect tag as soon as you conclude that there is bug. Many thanks for looking into it (and +1 for bisect – did you write a script to test the condition?) |
The difference is calling Currently, we have: periodic = box is not None At the crucial location. At first, this looks bad, since a user might want to avoid PBC even if they have box info. However, the value of box is actually verified by checking both that there are dimensions and that self.periodic is True by In the As far as I can tell, the first bad commit hash vs. its first ancestor diff does not show any changes to the core pbc flags default -- so my current hypothesis is that these are somehow being set to True by deafult and the activation of the pbc feature for sphzone selections is not necessarily wrong, but is a victim of default pbc activation flags? By the way, using the latest commit hash build of dev branch in IPython: In [1]: import MDAnalysis
In [2]: MDAnalysis.core.flags
Out[2]: [('use_periodic_selections', True), ('use_KDTree_routines', 'fast'), ('length_unit', 'Angstrom'), ('use_pbc', False), ('convert_lengths', True), ('force_unit', 'kJ/(mol*Angstrom)'), ('time_unit', 'ps'), ('speed_unit', 'Angstrom/ps'), ('charge_unit', 'e')] Unless I'm missing something, I propose that we write a unit test to verify that the default I've informally confirmed that using I note that none of this is commenting on the correctness of either selection from a geometric standpoint, simply addressing the perceived regression. |
Note that |
* Fixes Issue #1795 by setting the default value of use_periodic_selections flag to False. * Added corresponding unit test
From what I can tell, the bug here is that calling COG with pbc is wrong and doesn't do what you need. Eg for a molecule straddling the box boundaries, the COG is somewhere in the center of the box and has no relation to the position of the molecule. This is very similar to #1760 |
…ly handle regression in Issue #1795.
@tylerjereddy I had a quick look at this, I think this is still as issue this line shouldn't be using If I try and do the selection manually, I get 15 molecules, which is annoyingly neither of the previously reported answers.. g1 = u.select_atoms('resname WAT HOH')
g2 = u.select_atoms('resnum 99 147 231 261 289')
center = g2.center_of_geometry()
da = mda.lib.distances.distance_array(g1.positions, center)
len(np.where(da < 6.0)[0]) Unless I'm misunderstanding what a sphzone does I'll do some time travelling later to see where differences started happening... |
@richardjgowers I think that manual selection is correct. You are getting 15 atoms which is 5 water molecules. If you try something like:
you will get:
which is, I believe, correct answer. |
I call select_atoms method with following selection: '(resname WAT or resname HOH) and (sphzone 6.0 (resnum 99 147 231 261 289))'
Expected behaviour
I expect to get exactly the same results in 0.16.2 and 0.17.0 - 5 water molecules.
Actual behaviour
Version 0.16.2 gives 5 water molecules.
Version 0.17.0 gives 31 water molecules, completely different molecules.
The text was updated successfully, but these errors were encountered: