Fix check for tripolar Laplacian #123
Conversation
- test that northern edge grid data folds onto itself *only* away from land
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
=======================================
Coverage 98.41% 98.41%
=======================================
Files 9 9
Lines 944 944
=======================================
Hits 929 929
Misses 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
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! 🎄🎅🎄 |
|
Yes, let's finish this PR and the remaining issues related to the JOSS review in the first week of January. Happy holidays! |
|
Would anyone be willing to review this PR? |
gcm_filters/kernels.py
Outdated
| # note: grid data goes crazy for POP model land points so we don't want to check for land points | ||
| nx = np.shape(self.dxn)[-1] # number of longitudes or columns | ||
| # grab second to last row since we have already appended one extra row | ||
| first_half = (self.n_wet_mask * self.dxn)[..., [-2], : (nx // 2)] |
There was a problem hiding this comment.
It might be better to do this test with boolean indexing rather than multiplying by the wet mask. If, for example, some of the land data is NaN then I think this test would throw an error because 0 * np.NaN is np.NaN and np.NaN == np.NaN returns False. Maybe something like
first_half = self.dxn[...,-2,self.n_wet_mask[...,-2,:(nx//2)] == 1]
would avoid NaN problems.
There was a problem hiding this comment.
Thanks for the catch! In my latest commit I followed your suggestion - just using a different syntax that will also work with dask and cupy.
|
@iangrooms do you now approve this PR? If so I would like to merge this, so we can move forward with our JOSS revisions. |
Closes #122.
Problem description
The tripolar Laplacian
POPTripolarLaplacianTpointchecks 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...