Fix np.testing.assert_almost_equal usage #630
Conversation
Third argument to np.testing.assert_almost_equal should be integer number of decimals
| @@ -443,9 +443,9 @@ def compare_forces( | |||
| if compute_du_dx: | |||
| self.assert_equal_vectors(np.array(ref_du_dx), np.array(test_du_dx), rtol) | |||
| if compute_du_dl: | |||
| np.testing.assert_almost_equal(ref_du_dl, test_du_dl, rtol) | |||
| np.testing.assert_allclose(ref_du_dl, test_du_dl, rtol=rtol) | |||
There was a problem hiding this comment.
Do we want a rtol at all? Based on what is here: https://numpy.org/doc/stable/reference/generated/numpy.allclose.html it seems like rtol might be more flexible than we want? IE if the values are say 1,000,000 a relative tolerance of 1e-4 is 100, when we probably want to say that the values are always different by 0.001?
There was a problem hiding this comment.
Do we want a rtol at all?
Using atol instead makes the most sense to me for fixed-fixed comparisons, but also seems appropriate for fixed-floating like we have here. Maybe requiring both atol and rtol would make sense for fixed-floating?
mcwitt
force-pushed
the
fix-almost-equal-test
branch
from
33d8477 to
b1fae89
Compare
| if compute_du_dp: | ||
| np.testing.assert_almost_equal(ref_du_dp, test_du_dp, rtol) | ||
| np.testing.assert_allclose(ref_du_dp, test_du_dp, rtol=rtol, atol=atol) |
There was a problem hiding this comment.
nit: isclose(a, b, rtol=rtol) != isclose(b, a, rtol=rtol) in general -- the order of ref_ and test_ should probably be flipped in a few places
There was a problem hiding this comment.
Since this occurs in other files, and to not block the bug fix, I will address these all together in an upcoming PR. Created #635 to track
np.testing.assert_almost_equal expects a number of decimals to check as its 3rd argument, but we instead pass
rtolin tests fordu_dlanddu_dpinGradientTest.compare_forces.It appears that
decimalbetween 0 and 1 is interpreted as 0 rather than raising an error. For example, the following succeeds:To do