Skip to content

Clean up unit test for dscim.utils.utils.c_equivalence#135

Merged
kemccusker merged 4 commits intomainfrom
refactor_tests
Sep 7, 2023
Merged

Clean up unit test for dscim.utils.utils.c_equivalence#135
kemccusker merged 4 commits intomainfrom
refactor_tests

Conversation

@brews
Copy link
Copy Markdown
Member

@brews brews commented Aug 16, 2023

Cleans up unit test for dscim.utils.utils.c_equivalence().

  • Refactors the single test for dscim.utils.utils.c_equivalence() into multiple tests, with each test checking for a single behavior/test case.
  • The expected outcome for each test was precalculated and stored explicitly with some precision rather than calculated dynamically when tests are run. The expected values were created using the same math and code in the original test. For example, expected = (np.divide(array ** (1 - 10), 1 - 10).values.mean() * (1 - 10)) ** (1 / (1 - 10)) is now just expected = xr.DataArray(np.array(2.32634448)), which is the answer I get on my machine.
  • The test now checks against expected outcomes with xr.testing.assert_allclose(actual, expected) rather than assert expected == actual, as the original had done. This is helpful because == checks between floats can be difficult, if not impossible, and unreliable. Using xr.testing.assert_allclose allows for some "fuzziness".

@brews brews self-assigned this Aug 16, 2023
@brews
Copy link
Copy Markdown
Member Author

brews commented Aug 16, 2023

We're using xr.testing.assert_allclose with the default rtol=1e-05 and atol=1e-08. I'm not sure if we need to change how tight the tests are.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 16, 2023

Codecov Report

Merging #135 (a6ad90a) into main (95ec0de) will not change coverage.
Report is 9 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage   65.30%   65.30%           
=======================================
  Files          17       17           
  Lines        1842     1842           
=======================================
  Hits         1203     1203           
  Misses        639      639           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@brews brews marked this pull request as ready for review August 16, 2023 20:07
@brews brews requested a review from kemccusker August 16, 2023 20:07
@kemccusker kemccusker requested a review from JMGilbert August 22, 2023 18:55
Copy link
Copy Markdown
Member

@kemccusker kemccusker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good from my perspective, except for the minor update of the "normal eta" language

@kemccusker kemccusker merged commit 54ba269 into main Sep 7, 2023
@kemccusker kemccusker deleted the refactor_tests branch September 7, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants