BUG: Fixes #1795 sphzone selection regression

[tylerjereddy]

Fixes #1795

Supersedes attempted fix in #1806 based on feedback from @richardjgowers. The corresponding unit test now only crudely checks for regression of sphzone behavior as it relates to the reported issue, rather than making any claims about the geometric correctness of the selection.

I verified that the script I used for git bisecting the regression is happy with this fix & that the regression test (modified from the original unit test) does fail alongside the bisection script without the application of the fix used in this PR.

We'll see what the CI says. More detailed & correct handling of pbc & wrapping issues are delayed to future efforts, as discussed in the linked issue / PR above.

[tylerjereddy]

Looks like the new test fails in Python 2.7 but not 3.6; will have to investigate.

[tylerjereddy]

This is related to zip() but actually isn't repaired by using the six.moves.zip. I basically need a way to feed in parametrized arguments from subclassed test objects -- wasn't working when I tried to to do this early -- maybe I'll have another go tonight.

Basically, there are two test classes that inherit from BaseDistanceSelection, and the expected values are different in each case -- is there a straightforward way to feed in different values of the same Python object from the subclassed test scenarios to the parametrization decoration in test_spherical_zone?

This basically means that

 549     @pytest.mark.parametrize('input_val, expected',
 550                              list(zip(methods,
 551                                       sphzone_expected)))
 552     def test_spherical_zone(self, u, input_val, expected):

Should have different values of sphzone_expected (a list of 4 integer values) in different subclassed test cases -- should that be trivial? I feel like it should be, but actually a number of attempts failed!

[richardjgowers]

@tylerjereddy I don't think that's easily fixed. You could just redefine the test in each subclass

[richardjgowers]

So this means that kdtree and non-kdtree are returning a different result for periodic=True

[tylerjereddy]

Does that mean more changes are needed or we need to think about this a bit more?

[tylerjereddy]

Ok, I've redefined the pertinent test in each subclass with appropriate expected values. Locally, the tests now pass in Python 2.7 & 3.6 -- we'll see what the CI says.

The code duplication is perhaps a little ugly, but prettier solutions were proving problematic (and time consuming). A fixture or something probably could be used for concision.

[richardjgowers]

I'm looking into this myself, I think this fix is fine, but I want to double check it

[orbeckst]

@tylerjereddy I think you will have to look at the merge. This might also kick appveyor to actually run the tests.

[tylerjereddy]

@orbeckst Noted / put reminder on my calendar to take a look soon-ish

[tylerjereddy]

The test file has been refactored pretty substantially, AttributeError: type object 'BaseDistanceSelection' has no attribute 'methods'

I think there have been a lot distance-based operation changes lately -- has the linked issue been confirmed on latest develop branch? I'll have to check that next I guess.

[richardjgowers]

#1795 is now fixed