Use xarray.testing.assert_allclose

[TomNicholas]

Here we are using np.allclose over and over

roms-tools/roms_tools/tests/test_boundary_forcing.py

Line 396 in c1e89be

assert np.allclose(ds["zeta_south"].values, expected_zeta_south)

but we could just compare the whole dataset at once using xarray.testing.assert_allclose

[NoraLoose]

I'm generally all for using xr.testing.assert_allclose. There are a few reasons why I didn't do it here:

• I want to make sure that an error is raised if the dimension order is not s_, eta_, xi_ because this is the dimension order that ROMS assumes. xarray is insensitive to reordered dimensions.
• I'm actually not comparing the whole dataset because the computation of BoundaryForcing is terribly slow, even for a single time slice. (As is, the test is already taking up several minutes, which really slows down the entire test suite.) So I am skipping fields that are similar to others. This argument is more relevant here:
  

  roms-tools/roms_tools/tests/test_boundary_forcing.py

  Lines 717 to 723 in c1e89be

  	ds_bgc = bgc_boundary_forcing_from_climatology.ds
  	# Check the values in the dataset
  	assert np.allclose(ds_bgc["ALK_south"].values, expected_alk_south)
  	assert np.allclose(ds_bgc["ALK_east"].values, expected_alk_east)
  	assert np.allclose(ds_bgc["ALK_north"].values, expected_alk_north)
  	assert np.allclose(ds_bgc["ALK_west"].values, expected_alk_west)

  
  where I do not call ds.compute() before the comparison.

I'm hoping we can improve performance by working on #43 soon, and then the second point will be irrelevant.

[NoraLoose]

BTW do you know an easy way to write an entire expected_ds into the test?

When I have boundary_forcing.ds in a notebook and do print(boundary_forcing.ds) it just shows a condensed version like

I can't just copy / paste that.

[TomNicholas]

BTW do you know an easy way to write an entire expected_ds into the test?

No - I mean the Dataset could point to an arbitrarily large amount of data so it's truncated quickly. In general I'm not a fan of hardcoding expected data values into the testing code...

[NoraLoose]

But having these tests with expected data values has proven really, really useful when refactoring the code. These are the tests that have caught most of my bugs.

[NoraLoose]

At the same time they are the most annoying tests to maintain if you change small things 1) in the code (like a different interpolation method) or 2) in the test datasets that make the expected values change. But at least we can keep track of changes in the input fields to ROMS. Creating accurate input fields is the major purpose of ROMS-Tools (at least of roms_tools.setup), so I think within ROMS-Tools these tests are important.

[TomNicholas]

Then perhaps we should be saving very small output files as regression tests.

[NoraLoose]

Yes, that would definitely be better. I will work on this.