Last decimal place disagreements causing failed tests with numpy 1.22.0 or later

[lewisgross1296]

When testing from a clean copy of the develop branch of OpenMC (from the main fork) in debug mode, a few tests fail due to last decimal place or near it disagreements about values in a diff. The -v output from test_lethargy_bin_width shows one such example

test_lethargy_bin_width

    def test_lethargy_bin_width():
        f = openmc.EnergyFilter.from_group_structure('VITAMIN-J-175')
        assert len(f.lethargy_bin_width) == 175
        energy_bins = openmc.mgxs.GROUP_STRUCTURES['VITAMIN-J-175']
        assert f.lethargy_bin_width[0] == math.log10(energy_bins[1]/energy_bins[0])
>       assert f.lethargy_bin_width[-1] == math.log10(energy_bins[-1]/energy_bins[-2])
E       assert 0.05429280308859366 == 0.05429280308859367
E        +  where 0.05429280308859367 = <built-in function log10>((19640000.0 / 17332000.0))
E        +    where <built-in function log10> = math.log10

unit_tests/test_filters.py:257: AssertionError

This test and a few others fail for me when I was using python 3.10.5 and numpy 1.23.0. Did some digging with @pshriwise and we determined that with python 3.8.3 and numpy 1.21.0 these tests pass, but as soon as numpy is increased to or beyond 1.22.0, the assertions fail.

I was wondering if the appropriate fix would be to allow assert to vary with a certain tolerance (say like 1e-15) or if something else should be done with the environment to make sure they exactly agree.

[shimwell]

Perhaps replacing math.log10 with numpy.log10 for this test would also work

[lewisgross1296]

I'll try this locally and see if it passes

[lewisgross1296]

@shimwell Looks like that did the trick for test_lethargy_bin_width. Just out of curiosity I grep'd for more math. and these came up. Do you think it would be worth PRing to change all these to numpy if a similar numpy function exists? This assumes numpy is always better to use and has an equivalent function, so maybe there reasons to keep some of these as math. ?

At the least, I can PR to change this one test.

lgross@ulam:~/openmc (lethargy_test_fix) $ grep -rI "= math."
tests/unit_tests/test_surface.py:    psi = math.degrees(math.atan2(1, 2))
tests/unit_tests/test_surface.py:    phi = math.degrees(math.atan2(1, math.sqrt(5)))
tests/unit_tests/test_surface.py:    phi = math.degrees(math.atan2(1, math.sqrt(2)))
tests/regression_tests/deplete_with_transport/example_geometry.py:    v_fuel = math.pi * r_fuel**2
tests/regression_tests/deplete_with_transport/example_geometry.py:    v_gap = math.pi * r_gap**2 - v_fuel
tests/regression_tests/deplete_with_transport/example_geometry.py:    v_clad = math.pi * r_clad**2 - v_fuel - v_gap
tests/regression_tests/deplete_with_transport/example_geometry.py:        r_rings[i] = math.sqrt(1.0/(math.pi) * v_ring * (i+1))
openmc/deplete/chain.py:                decay_constant = math.log(2) / nuc.half_life
openmc/deplete/chain.py:            level = math.inf
openmc/surface.py:    cx, sx = math.cos(phi), math.sin(phi)
openmc/surface.py:    cy, sy = math.cos(theta), math.sin(theta)
openmc/surface.py:    cz, sz = math.cos(psi), math.sin(psi)
vendor/xtensor/include/xtensor/xmath.hpp:                auto d = math::abs(internal_type(a) - internal_type(b));
vendor/xtensor/include/xtensor/xcomplex.hpp:        using functor = math::conj_impl_fun;
vendor/xtensor/include/xtensor/xcomplex.hpp:        using functor = math::arg_fun;
vendor/xtensor/include/xtensor/xcomplex.hpp:        using functor = math::norm_fun;
vendor/xtensor/include/xtensor/xio.hpp:                        m_max = math::abs(val);
vendor/xtensor/include/xtensor/xio.hpp:                    m_max = math::abs(val);

EDIT: looks like some of these actually shouldn't be changed. Like the vendor ones for sure shouldn't. But the tests potentially could

[shimwell]

Switching lethargy test to numpy worked because the lethargy function uses numpy. Not sure about the others, I just happened to have written the lethargy function.

[paulromano]

maybe there reasons to keep some of these as math

@lewisgross1296 If you are evaluating the function on a scalar value, it is generally preferable to use math.func(...) instead of numpy.func(...) because the latter has to do extra work to check the type of its argument (to differentiate between an array and a scalar), whereas math.func(...) always assumes its argument scalar and returns a scalar.

[lewisgross1296]

Gotcha, to clarify, would this be a case where math.log10 is better?

a = numpy.log10(array[-1])

Or is numpy better since it is doing an operation involving an array? In my head, array[-1] is a scalar, but maybe I'm misunderstanding.

[paulromano]

If array[-1] is a scalar, I would pass it to math.log10

[paulromano]

Closed by #2193

[lewisgross1296]

There actually were other tests that were failing when using numpy 1.22.0 or later but passing with 1.21.0 or earlier. This was just a good example since there was only one comparison in the diff. I can always revert my numpy version, but I'd think we eventually will upgrade?

[paulromano]

Ah, sorry. Yes, numpy changed something in 1.22 that results in numbers being displayed slightly differently. The easiest thing would be for us to just update the failing tests and force anyone who wants to test locally to update to 1.22+

[kkiesling]

I think I have been seeing this same issue with tests on a server and just confirmed that I am indeed using numpy 1.23.2. Is the current recommendation to just accept theses floating point errors in the tests or to bump numpy down to 1.21 for reliable usage? (this may be a vague question without additional context)

[lewisgross1296]

For the purpose of having my local tests pass, I just downgraded to 1.21.0. I do think we want to eventually upgrade our numpy version though.

I'm guessing your GitHub Actions tests would still pass when you push to a PR. I think since that is the requirement for merging, it is up to you whether to downgrade wherever your tests are running.

[kkiesling]

I am not using GH actions, but I am trying to get the newest version up and running on a cluster for people and haven't been able to get the tests to pass. I think this is the reason. I will downgrade and see if that helps.