NOAA-GFDL / xwmt

Python package for water mass transformation analysis that leverages xarray functionality
https://xwmt.readthedocs.io/
MIT License
7 stars 5 forks source link

Simple test suite needed #10

Open jkrasting opened 2 years ago

jkrasting commented 2 years ago

We would benefit from a simple test suite invoked via pytest.

gmacgilchrist commented 2 years ago

I started putting together what I imagined a test suite might look like when I was messing around with alternative configurations of the package here.

jetesdal commented 2 years ago

@gmacgilchrist The link does not work for me. Is it a private repository?

jetesdal commented 2 years ago

Some of this is now addressed with #17

hdrake commented 1 year ago

@gmacgilchrist, @jetesdal, and @jkrasting, what do you think about replacing the existing testing suite (which indexes to past xwmt results) with the comparisons to analytical solutions described in https://github.com/NOAA-GFDL/xwmt/issues/39#issuecomment-1531722015, e.g. by testing thatxwmt's errors relative to analytical solutions are small and converge as dz -> 0?

jkrasting commented 1 year ago

The idea has merit, @hdrake. As long it covers the same code (if not more), I'm on board.

gmacgilchrist commented 1 year ago

I'm a big fan of testing against the analytical solutions. I think they should sit alongside tests that perform the xwmt calculations on more comprehnsive 2d and 3d datasets (e.g. one timestep of the Baltic test case). Tests against the analytical solution will ensure that the wmt calculation continues to be correct, while tests against known answers for a full, complex dataset will ensure that changes to the code don't break anything associated with working with the gridded data.

hdrake commented 1 year ago

Tests against the analytical solution will ensure that the wmt calculation continues to be correct, while tests against known answers for a full, complex dataset will ensure that changes to the code don't break anything associated with working with the gridded data.

I'm coming around to this, with the crucial caveat that those tests could very well fail due to future developments, not because we break something in the code, but because the hard-coded values currently there are wrong (i.e. if there are bugs in the current code). That said, this is less concerning to me now that I have convinced myself the current code converges on the analytical solutions in a variety of configurations.

hdrake commented 1 year ago

pytest suite that checks convergence with analytical solutions is now implemented in https://github.com/hdrake/xwmt/commit/e1e0f830994dcf15f504dfea5dbdb48a93e06a46

hdrake commented 1 year ago

Some of this is now addressed with #17

FYI, the testing datasets used in test_functional.py and test_functional_3d.py don't actually seem to be in the repository. I'm guessing that is because they are rather large and should not be version controlled along with the code at risk of inflating the package? Do we need to include some lines in the test scripts that scape the file from somewhere online or something?

gmacgilchrist commented 1 year ago

I'm coming around to this, with the crucial caveat that those tests could very well fail due to future developments, not because we break something in the code, but because the hard-coded values currently there are wrong (i.e. if there are bugs in the current code).

I appreciate this concern. However, given the relative simplicity of what the transformation calculation actually does (transform coordinates then integrate terms), I think we can be fairly confident in having a baseline "right answer" to test against.

FYI, the testing datasets used in test_functional.py and test_functional_3d.py don't actually seem to be in the repository. I'm guessing that is because they are rather large and should not be version controlled along with the code at risk of inflating the package? Do we need to include some lines in the test scripts that scape the file from somewhere online or something?

@jetesdal, what is your current approach for setting up the testing datasets?

hdrake commented 1 year ago

I appreciate this concern. However, given the relative simplicity of what the transformation calculation actually does (transform coordinates then integrate terms), I think we can be fairly confident in having a baseline "right answer" to test against.

I'm not as concerned about the mechanics of the transformation step itself as I am the degree to which discretization errors (and formal blow-up of water mass transformations when gradients vanish) affect our interpretation of them. The package could very well produce technically correct results but then be meaningless if they are dominated by "discretization" errors, which we still are not able to convincingly separate from numerical mixing. I agree these tests are a useful protection against breaking changes in the core lambda-binning operation, but I would not read much further into it than that.

[Even then, I'll note that I had to switch the method of interpolation of lambda to interfaces from boundary=extrapolate to boundary=extend, as the former had been deprecated by xgcm. It is not obvious which of these (if either) is correct, and they could plausibly give different/incorrect answers in edge cases, so I'm not even that convinced that the transforming operation is that robust.]

jetesdal commented 1 year ago

@jetesdal, what is your current approach for setting up the testing datasets?

I think that @jkrasting set up the testing data files on an FTP site. I can see that there is the following in xwmt/.github/workflows/ci.yml for getting the data files:

- name: Download test data
  run: |
    curl -o xwmt_test_data.nc ftp://ftp.gfdl.noaa.gov/perm/John.Krasting/xwmt/xwmt_test_data.20220810.nc

Running the above curl command should download the data, but I am unsure if this includes the latest data set we used in test_functional.py (xwmt_test_data.nc) and test_functional_3d.py (xwmt_test_data_baltic_3d.nc). @jkrasting, as far as I remember we put the 3d testing data also on the ftp site. Do we need to update the link in the CI workflow?

jkrasting commented 1 year ago

@jetesdal is correct. There is a 3D test dataset on the FTP site: xwmt_test_data_baltic_3d.20221012.nc