ocean-eddy-cpt / gcm-filters

Diffusion-based Spatial Filtering of Gridded Data

https://gcm-filters.readthedocs.io/

Other

37 stars 24 forks source link

Fix check for tripolar Laplacian #123

Closed NoraLoose closed 2 years ago

NoraLoose commented 2 years ago

Closes #122.

Problem description

The tripolar Laplacian POPTripolarLaplacianTpoint checks that the northern edge grid data folds onto itself - a necessary criterion of the tripole grid geometry. However, this check failed when this Laplacian got applied to some actual POP grid data (rather than just our synthetic test data in our package), such as in our example notebook in the documentation.

Solution

I changed the check to only test along northern edge grid data that is not on land. The POP grid data goes a little bit crazy on land points--and does whatever--, so we don't want to check on land. The check now passes.

Lesson learned

This brings us back to an old question in #5: Should we do our tests on real model data, or at least, real model grid data (rather than synthetic data)? We would have discovered this issue much earlier...

codecov-commenter commented 2 years ago

Codecov Report

Merging #123 (25bb409) into master (7898c9c) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files           9        9           
  Lines         944      944           
=======================================
  Hits          929      929           
  Misses         15       15

Flag	Coverage Δ
unittests	`98.41% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/test_kernels.py	`100.00% <ø> (ø)`
gcm_filters/kernels.py	`98.56% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7898c9c...25bb409. Read the comment docs.

rabernat commented 2 years ago

Hi Nora! Thanks for working on this important issue. Unfortunately I am about to unplug for my holiday break. I will be back online the first week of January and will try to review this then. If you want to move faster, then you don't have to wait for me. On the other hand, I hope you (and everyone else involved) is also planning to take a break as well! 🎄🎅🎄

NoraLoose commented 2 years ago

Yes, let's finish this PR and the remaining issues related to the JOSS review in the first week of January. Happy holidays!

NoraLoose commented 2 years ago

Would anyone be willing to review this PR?

NoraLoose commented 2 years ago

@iangrooms do you now approve this PR? If so I would like to merge this, so we can move forward with our JOSS revisions.