NanoComp / meep

free finite-difference time-domain (FDTD) software for electromagnetic simulations
GNU General Public License v2.0
1.19k stars 610 forks source link

Replace obsolete routine `compare_arrays` with `assertClose` in MPB unit tests #2547

Closed oskooi closed 1 year ago

oskooi commented 1 year ago

The MPB unit tests in test_mpb.py are currently using numpy.testing.assert_allclose and compare_arrays (added in #345). For comparing two arrays, these functions have been replaced with assertClose which is used in all the other tests.

This PR updates test_mpb.py to only use assertClose. This modification should hopefully make this test less flaky (it is currently failing the CI). Also, compare_arrays is removed from utils.py since it is not being used anywhere else. Finally, type hints are added to assertClose and its docstrings are updated.

stevengj commented 1 year ago

How does assertClose work for arrays? One of the major reasons we are using compare_arrays is because the Python array-comparison methods are element-wise rather than using array norms, which made it impossible to reliably specify a relative error.

(Basically, I think Python's isclose and allclose are broken.)

oskooi commented 1 year ago

How does assertClose work for arrays? One of the major reasons we are using compare_arrays is because the Python array-comparison methods are element-wise rather than using array norms, which made it impossible to reliably specify a relative error.

assertClose is the custom array-comparator function which uses the array norm to define the relative error that we added as part of #1569:

https://github.com/NanoComp/meep/blob/eb8ff3aff135f28e5171e2417817b6bbd9c98e7f/python/tests/utils.py#L20-L30

I suspect the current use of compare_arrays and numpy.testing.assert_allclose in the MPB unit tests are why this test is so flaky (see recent failure). This PR seems to improve things.

oskooi commented 1 year ago

Inspecting the log of the failing test_mpb.py instance, the cause of the error is numpy.testing.assert_allclose (and not compare_arrays):

======================================================================
FAIL: test_get_eigenvectors (__main__.TestModeSolver)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/meep/meep/build/meep-1.28.0-beta/_build/sub/python/../../../python/tests/test_mpb.py", line 1609, in test_get_eigenvectors
    compare_eigenvectors("tutorial-te-eigenvectors.h5", 1, 8)
  File "/home/runner/work/meep/meep/build/meep-1.28.0-beta/_build/sub/python/../../../python/tests/test_mpb.py", line 1606, in compare_eigenvectors
    np.testing.assert_allclose(expected, ev, rtol=1e-3)
  File "/opt/hostedtoolcache/Python/3.10.11/x64/lib/python3.10/site-packages/numpy/testing/_private/utils.py", line 1592, in assert_allclose
    assert_array_compare(compare, actual, desired, err_msg=str(err_msg),
  File "/opt/hostedtoolcache/Python/3.10.11/x64/lib/python3.10/contextlib.py", line 79, in inner
    return func(*args, **kwds)
  File "/opt/hostedtoolcache/Python/3.10.11/x64/lib/python3.10/site-packages/numpy/testing/_private/utils.py", line 862, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Not equal to tolerance rtol=0.001, atol=0

This is perhaps not surprising since numpy.testing.assert_allclose tries to enforce equivalence among all elements of the two arrays and fails if any one element disagrees. At a minimum, we can therefore simply replace the one instance of this function in test_mpb.py with assertClose.

However, it is not clear why we need to have both compare_arrays and assertClose since they perform the same function using arrays norms yet in a slightly different way.