giovannipizzi/seekpath

Failed to pass the run_tests.py.

hongyi-zhao opened this issue · 2 comments

Hi,

I install the git master version of seekpath will pip's editable mode with the following steps:

# Doing this within pyenv using python 3.8.1:
$ pyenv shell 3.8.1
# Create a virtualenv named as hpc for this test:
$ pyenv virtualenv hpc
$ pyenv shell hpc 
$ git clone https://github.com/giovannipizzi/seekpath.git seekpath.git
$ cd seekpath.git
$ pip install -e .

And I also installed the all of the packages listed in the *requirements.txt files.
Then I do the following testing:

$ python run_tests.py 
ssF.ssss..................................................................
======================================================================
FAIL: test_2 (seekpath.brillouinzone.test_brillouinzone.TestBZ)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/werner/Public/hpc/tools/seekpath.git/seekpath/brillouinzone/test_brillouinzone.py", line 254, in test_2
    self.assertTrue(is_same,
AssertionError: False is not true : The two sets of faces are different: The following item in the first list was not found in the second one: [[-0.5, 0.5, -0.3400000000000001], [0.30000000000000004, 0.5, -0.4999999999999999], [0.3, 0.3, -0.6599999999999999], [-0.5, 0.3, -0.4999999999999999]]

----------------------------------------------------------------------
Ran 74 tests in 0.340s

FAILED (failures=1, skipped=6)

Any hints for this issue?

Regards

Thanks for reporting!
I think the code is correct, but the tests have a small issue, that should be fixed by this small change:

diff --git a/seekpath/brillouinzone/test_brillouinzone.py b/seekpath/brillouinzone/test_brillouinzone.py
index 05ea73e..962602c 100644
--- a/seekpath/brillouinzone/test_brillouinzone.py
+++ b/seekpath/brillouinzone/test_brillouinzone.py
@@ -59,7 +59,7 @@ def is_same_face(f1, f2):
     if found:
         return True
     # Try also reversed order
-    indices = (-(np.arange(len(f2)) + shift)) % len(f2)
+    indices = (-np.arange(len(f2)) + shift) % len(f2)
     err = np.abs(np.array(f1) - np.array(f2)[indices]).mean()
     found = err < threshold
     return found

I had thought about the possibility that the order of the faces was random, and also that a face could be returned with points in reversed order (i.e., upside down); but the test to check this case had a bug (that probably was never spotted because until now the order was the same as in my test).

Since I cannot reproduce your results at the moment, could you check if this change makes the tests pass? Great if you could do the full test, or at least test the following python code:

import numpy as np
from seekpath.brillouinzone import test_brillouinzone as tbz

expected_face = [
    [-0.5, 0.3, -0.5],
    [0.3, 0.3, -0.66],
    [0.3, 0.5, -0.5],
    [-0.5, 0.5, -0.34]]

res = [
    [-0.5, 0.5, -0.3400000000000001],
    [0.30000000000000004, 0.5, -0.4999999999999999],
    [0.3, 0.3, -0.6599999999999999],
    [-0.5, 0.3, -0.4999999999999999]]


def is_same_face_new(f1, f2):
    """
    Check if two faces are the same.

    f1 and f2 should be a list of 3d points (i.e., a list of lists).

    It also checks if the the coordinates are shifted (i.e. the first one is 
        now the second, the second the third etc.). Tries also points in 
        reversed order.
    """
    threshold = 1.e-7

    # Must have same # of points
    if len(f1) != len(f2):
        return False

    # Empty faces are the same
    if len(f1) == 0:
        return True

    found = False
    # Find (first occurrence of) first point
    for shift, p2 in enumerate(f2):
        if tbz.is_same_point(f1[0], p2):
            found = True
            break

    if not found:
        return False

    # make a new version of f2 with shifted indices, and check
    # now if all points are the same
    indices = (np.arange(len(f2)) + shift) % len(f2)
    err = np.abs(np.array(f1) - np.array(f2)[indices]).mean()
    found = err < threshold
    if found:
        return True
    # Try also reversed order
    indices = (-np.arange(len(f2)) + shift) % len(f2)
    err = np.abs(np.array(f1) - np.array(f2)[indices]).mean()
    found = err < threshold
    return found


def is_same_face_old(f1, f2):
    """
    Check if two faces are the same.

    f1 and f2 should be a list of 3d points (i.e., a list of lists).

    It also checks if the the coordinates are shifted (i.e. the first one is 
        now the second, the second the third etc.). Tries also points in 
        reversed order.
    """
    threshold = 1.e-7

    # Must have same # of points
    if len(f1) != len(f2):
        return False

    # Empty faces are the same
    if len(f1) == 0:
        return True

    found = False
    # Find (first occurrence of) first point
    for shift, p2 in enumerate(f2):
        if tbz.is_same_point(f1[0], p2):
            found = True
            break

    if not found:
        return False

    # make a new version of f2 with shifted indices, and check
    # now if all points are the same
    indices = (np.arange(len(f2)) + shift) % len(f2)
    err = np.abs(np.array(f1) - np.array(f2)[indices]).mean()
    found = err < threshold
    if found:
        return True
    # Try also reversed order
    indices = (-(np.arange(len(f2)) + shift)) % len(f2)
    err = np.abs(np.array(f1) - np.array(f2)[indices]).mean()
    found = err < threshold
    return found

if __name__ == "__main__":
    print("Old result:", is_same_face_old(expected_face, res))
    print("New result:", is_same_face_new(expected_face, res))
    print("Old result:", is_same_face_old(res, expected_face))
    print("New result:", is_same_face_new(res, expected_face))

If you confirm that this fix works, I'll make a new release fixing this. Thanks!

Testing on the shipped test script:

$ python run_tests.py 
ss..ssss..................................................................
----------------------------------------------------------------------
Ran 74 tests in 0.549s

OK (skipped=6)

With miniconda-latest + python version 3.8.2, I got the following warning:

$ python run_tests.py 
ss<frozen importlib._bootstrap>:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
<frozen importlib._bootstrap>:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
<frozen importlib._bootstrap>:219: RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 from PyObject
..ssss..................................................................
----------------------------------------------------------------------
Ran 74 tests in 0.371s

OK (skipped=6)

Why some tests are skipped?

Testing on the above python code by saving them in a file named as bug-test.py:

$ python bug-test.py 
Old result: False
New result: True
Old result: False
New result: True

BTW, though I gives the test results here, but I still not so clear about the meanings on them. So, please check them based on your codes.

Regards