Skip to content
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

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

Merged
merged 2 commits into from
Jun 22, 2023

Conversation

oskooi
Copy link
Collaborator

@oskooi oskooi commented Jun 11, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

oskooi commented Jun 11, 2023

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:

class ApproxComparisonTestCase(unittest.TestCase):
"""A mixin for adding proper floating point value and vector comparison."""
def assertClose(self, x, y, epsilon=1e-2, msg=""):
"""Asserts that two values or vectors satisfy ‖x-y‖ ≤ ε * max(‖x‖, ‖y‖)."""
x = np.atleast_1d(x).ravel()
y = np.atleast_1d(y).ravel()
x_norm = np.linalg.norm(x, ord=np.inf)
y_norm = np.linalg.norm(y, ord=np.inf)
diff_norm = np.linalg.norm(x - y, ord=np.inf)
self.assertLessEqual(diff_norm, epsilon * np.maximum(x_norm, y_norm), msg)

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
Copy link
Collaborator Author

oskooi commented Jun 11, 2023

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.

@stevengj stevengj merged commit 0aabdcc into NanoComp:master Jun 22, 2023
@oskooi oskooi deleted the compare_arrays_replace branch June 22, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants