write a test for spherical selections tokens using the periodic kdtree #1733

jmborr · 2017-12-11T19:32:58Z

IN PROGRESS

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

richardjgowers · 2017-12-12T10:48:34Z

This looks like it'll be a good addition. If we can get the kdtree version of all selections working, we should probably make that the only option.

jmborr · 2017-12-12T13:39:22Z

@richardjgowers I just opened issue #1736 to test cylindrical selections 😄

jmborr · 2017-12-12T23:01:16Z

@orbeckst @richardjgowers I'm running into a bit of a problem here. test_atomselections.py uses trajectory file adk_dims.dcd which when loaded results in dimensions=[0, 0, 0, 90, 90, 90]. Two distance-based selection tests taking into account periodic boundary conditions are failing because of this.
Do you know how can one end up with such bizarre dimensions?

orbeckst · 2017-12-12T23:04:06Z

The trajectory was simulated without PBC. If you need a trajectory with PBC, use TPR/XTC.

You could fudge the PSF/DCD test by setting the dimensions to some reasonable value before doing the selections. (u.atoms.bbox() might help to get a tight box)

Incidentally, failing with undefined box is a good corner case to catch.

jmborr · 2017-12-12T23:28:03Z

@orbeckst So do I understand that if I load a trajectory with no PBC, MDAnalysis will always assign dimensions=[0, 0, 0, 90, 90, 90] instead of dimensions=None or is this just some bizarre occurrence?

orbeckst · 2017-12-12T23:44:06Z

Yes, that's what we put when there is no box.

jmborr · 2017-12-12T23:48:51Z

OK, then I'll insert a box checking to prevent the combination of dimensions=[0, 0, 0, 90, 90, 90] and the default MDAnalysis.core.flags['use_periodic_selections']=True to end up fooling DistanceSelection.apply()

richardjgowers · 2017-12-12T23:55:39Z

Isn't this more a quirk of the topology file saying the box is 0,0,0 rather than Mda saying it? I'll look into this before we uglify the code with checks

…

On Tue, 12 Dec 2017, 11:48 p.m. Jose Borreguero, ***@***.***> wrote: OK, then I'll insert a box checking to prevent the combination of dimensions=[0, 0, 0, 90, 90, 90] and the default MDAnalysis.core.flags['use_periodic_selections']=True to end up fooling DistanceSelection.apply() — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jBxbyRd3R8VL7a86pbXfuK7YNRqUwks5s_xDjgaJpZM4Q96qU> .

orbeckst · 2017-12-13T00:11:40Z

The topology says nothing about the box. The trajectory has box information – or not. However...

I checked for our XYZ example, which does not contain box information, and I get dimensions == array([ 0., 0., 0., 0., 0., 0.], dtype=float32).

Thus, it is not true that we always set the box to [0, 0, 0, 90, 90, 90] when we don't have the box. Sorry @jmborr , I was wrong. It seems that we are not handling this case uniformly and have left it to the reader implementation. For example, the DCD reader code initializes lengths to 0 and angles to 90º

mdanalysis/package/MDAnalysis/lib/formats/libdcd.pyx

Line 581 in eb3678d

unitcell[0] = unitcell[2] = unitcell[5] = 0.0;

whereas the XYZ reader just uses the base defaults (all 0).

Barring a comprehensive survey, I am assuming that having no box information will at least lead to

all(dimensions[:3] == 0) == True

(or the float equivalent np.allclose())

orbeckst · 2017-12-13T00:19:27Z

(I opened #1738 so that this point is not forgotten.)

richardjgowers · 2017-12-13T17:01:05Z

I think the hack here for dimensions is fine, and then one day in the future we'll have nicer handling that is universal across the package

jmborr · 2017-12-15T16:44:48Z

@orbeckst @richardjgowers I'm having an error with one of the builds that seems unrelated to the code changes. Have you ever encountered something similar?

orbeckst · 2017-12-15T17:35:08Z

I've seen this one before. For some reason, on the Mac OSX workers, pytest sometimes fails to create a temp directory; possibly an issue with the xdist plugin for running in parallel (??). Not quite sure. (Or maybe we wrote an incorrect pytest fixture somewhere.)

I restarted https://travis-ci.org/MDAnalysis/mdanalysis/jobs/315961274 , maybe it goes away...

orbeckst · 2018-01-12T18:43:34Z

I restarted the Mac OS X travis job again, but if it fails again, I'd be willing to ignore it.

orbeckst

Minor changes as far as I can see.

Maybe @richardjgowers or @kain88-de can also have a quick look?

orbeckst · 2018-01-12T18:49:08Z

package/MDAnalysis/core/selection.py

+        Returns
+        -------
+        None or numpy.ndarray
+            Returns argument dimensions if system is periodic, otherwise


This description "if system is periodic" is ambiguous because self.periodic is an external choice if periodicity ought to be taken into account. Can you please say more clearly what goes into deciding if None or the box is returned?

Also make clear that we only consider full 3D periodicity as periodic, we don't support periodicity in only some dimensions (or arbitrary space group operators... that would be fun).

Then the wording can be changed "periodic in all three dimensions" and the validation code can be changed to:

if self.periodic and all(dimensions[:3]): return dimensions return None

What do you think?

Yes, sounds good.

orbeckst · 2018-01-12T18:51:05Z

package/MDAnalysis/core/selection.py

-        ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32)
-
-        box = group.dimensions if self.periodic else None
+        periodic = False if self.validate_dimensions(group.dimensions) is None\


shorter

periodic = self.validate_dimensions(group.dimensions) is not None

But even better, rearrange your code to avoid calling the same method twice:

box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry(pbc=periodic).reshape(1, 3).astype(np.float32)

orbeckst · 2018-01-12T18:51:18Z

package/MDAnalysis/core/selection.py

-        ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32)
-
-        box = group.dimensions if self.periodic else None
+        periodic = False if self.validate_dimensions(group.dimensions) is None\


shorter

periodic = self.validate_dimensions(group.dimensions) is not None

But even better, rearrange your code to avoid calling the same method twice:

box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry(pbc=periodic).reshape(1, 3).astype(np.float32)

orbeckst · 2018-01-12T21:51:42Z

Please also update CHANGELOG and docs - probably somewhere under selections.

jmborr · 2018-01-13T21:46:22Z

@orbeckst One test is failing because of errors at the 12th decimal place. Is this accuracy unacceptable?

orbeckst · 2018-01-13T23:53:01Z

Generally, we're ok with accuracy to single (float32) precision, which is about 6-7 decimals. Double is nice to have but I don't have a problem if you decrease the number of decimals for the test. If it passes at 10 decimals then that's still good.

…

-- Oliver Beckstein email: orbeckst@gmail.com

Am Jan 13, 2018 um 14:46 schrieb Jose Borreguero ***@***.***>: @orbeckst One test is failing because of errors at the 12th decimal place. Is this accuracy unacceptable? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jmborr · 2018-01-15T20:42:23Z

@orbeckst There seem to be a conflict in the python 3.4 build because the numpy requirement is incompatible with this python version:

UnsatisfiableError: The following specifications were found to be in conflict:
  - numpy 1.14* -> python >=2.7,<2.8.0a0
  - python 3.4*
Use "conda info <package>" to see the dependencies for each package.

I guess this will affect all pull requests

kain88-de · 2018-01-15T21:00:11Z

@jmborr I'll fix this on the build server sometime this week. Anything else effected by this PR?

jmborr · 2018-01-15T21:04:13Z

@kain88-de If you want, you could take a look to this Mac OS-X build flagged as "allowed to fail".

kain88-de · 2018-01-15T21:07:21Z

yeah, that is expected. No idea what is wrong with OSX right now. None of us has the time to look into it currently. Therefore it is set to be allowed to fail.

jmborr · 2018-01-15T21:20:27Z

I travel in the same tight ship, so that's fine with me 😸

kain88-de · 2018-01-15T21:23:48Z

You can restart the python 3.4 test if this PR is merged. conda-forge/numpy-feedstock#76

jmborr · 2018-01-19T16:02:56Z

@kain88-de is the conda-forge/numpy-feedstock#76 PR on ice ?

kain88-de · 2018-01-19T16:14:17Z

Travis is on ice for the last couple days. They still struggle with OS X builds. We are trying to get those to pass. That will likely still take some time.

…

On 19. Jan 2018, at 17:02, Jose Borreguero ***@***.***> wrote: @kain88-de is the conda-forge/numpy-feedstock#76 PR on ice ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jmborr · 2018-01-19T16:25:13Z

@kain88-de I wasn't aware Travis was having such an issue, good to know 👍

orbeckst added Component-Selections performance testing Work in progress labels Dec 11, 2017

orbeckst mentioned this pull request Dec 13, 2017

consistently indicate absence of box information Timestep.dimensions #1738

Closed

orbeckst requested changes Jan 12, 2018

View reviewed changes

jmborr added 6 commits January 12, 2018 17:40

Refs MDAnalysis#1731 Spherical zone selection test

4baa8a9

Refs MDAnalysis#1731 Spherical layer selection test

56c4a7c

Refs #21367 Validate dimensions

51b6270

Refs MDAnalysis#1731 Refactor box and periodic definition

f9e99df

Refs MDAnalysis#1731 Refactor validate_dimensions

7531c8f

Refs MDAnalysis#1731 resolve conflicts

ff7f4d9

jmborr force-pushed the 1731_spherical_tokens_pkdtree branch from e295452 to ff7f4d9 Compare January 12, 2018 22:44

Refs MDAnalysis#1731 fix accuracy of failing test

8ef999c

orbeckst approved these changes Jan 19, 2018

View reviewed changes

orbeckst merged commit b837af1 into MDAnalysis:develop Jan 21, 2018

This was referenced Jan 22, 2018

test_AngularDistribution failure #1756

Closed

made tests for waterdynamics more robust and allow Python 3.4 to fail #1757

Merged

jmborr deleted the 1731_spherical_tokens_pkdtree branch January 22, 2018 18:17

jmborr mentioned this pull request Mar 13, 2018

add kdtree option for spherical selection keywords #1706

Open

write a test for spherical selections tokens using the periodic kdtree #1733

write a test for spherical selections tokens using the periodic kdtree #1733

Conversation

jmborr commented Dec 11, 2017 • edited by orbeckst

PR Checklist

richardjgowers commented Dec 12, 2017

jmborr commented Dec 12, 2017

jmborr commented Dec 12, 2017 • edited

orbeckst commented Dec 12, 2017 • edited

jmborr commented Dec 12, 2017

orbeckst commented Dec 12, 2017

jmborr commented Dec 12, 2017

richardjgowers commented Dec 12, 2017 via email

orbeckst commented Dec 13, 2017

orbeckst commented Dec 13, 2017

richardjgowers commented Dec 13, 2017

jmborr commented Dec 15, 2017

orbeckst commented Dec 15, 2017

orbeckst commented Jan 12, 2018

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Jan 12, 2018

jmborr commented Jan 13, 2018

orbeckst commented Jan 13, 2018 via email

jmborr commented Jan 15, 2018

kain88-de commented Jan 15, 2018

jmborr commented Jan 15, 2018

kain88-de commented Jan 15, 2018

jmborr commented Jan 15, 2018

kain88-de commented Jan 15, 2018

jmborr commented Jan 19, 2018

kain88-de commented Jan 19, 2018 via email

jmborr commented Jan 19, 2018

jmborr commented Dec 11, 2017 •

edited by orbeckst

jmborr commented Dec 12, 2017 •

edited

orbeckst commented Dec 12, 2017 •

edited