Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter verification tests #153

Merged
merged 4 commits into from
Jun 8, 2022

Conversation

NoraLoose
Copy link
Member

This PR adds verification tests for the filter algorithm, in preparation for the refactor in #149, which replaces the filter algorithm with a new one.

This PR essentially mirrors #79, but here the verification tests are associated with the test_filter.py module (rather than the test_kernels.py module as in #79). The main changes are:

  • I moved fixture definitions from tests/test_filter.py to tests/conftest.py and, in the process, got rid off a lot of redundant code.
  • I added the actual verification tests via test_filter_validation.py.

- in the old version, grid scale and filter scale were chosen as
  equal; filtering does not really do anything in that case
@NoraLoose NoraLoose requested a review from rabernat May 10, 2022 00:03
Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Overall this look like a great improvement! Big 👍 to PRs that remove code! 😄

I just have one question to make sure I understand what is happening here.

da_v = xr.DataArray(data_v, dims=["y", "x"])

return grid_type, da_u, da_v, grid_vars, geolat_u

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just help me understand why all this code is being removed? Is it redundant with the code in conftest.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. The only two differences between the removed code and the code in conftest.py were:

  1. conftest.py created numpy array test data (specific to test_kernels.py), whereas the removed code created xarray DataArray test data (specific to test_filter.py). The new fixtures grid_type_and_input_ds and vector_grid_type_and_input_ds in conftest.py take care of the numpy array--> xarray DataArray conversion, but without all the redundancy.
  2. The random number generator for the test data. Using the explicit constant seed for the random number generator in conftest.py ensures that we are always testing on the same data - a pre-requisite for the filter verification tests, exactly as done in Refactor kernel tests #79.

@NoraLoose
Copy link
Member Author

@rabernat do you agree that we can merge this PR?

@rabernat
Copy link
Contributor

rabernat commented Jun 8, 2022

Thanks for the ping Nora! Yes, absolutely, I will do it now.

@rabernat rabernat merged commit b958e92 into ocean-eddy-cpt:master Jun 8, 2022
@NoraLoose NoraLoose deleted the verification-filter-tests branch June 8, 2022 22:00
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.

2 participants