Partial Collapse of Multidimensional Auxcoords with Bounds#3302
Partial Collapse of Multidimensional Auxcoords with Bounds#3302lbdreyer merged 3 commits intoSciTools:masterfrom
Conversation
rcomer
force-pushed
the
partial-collapse-bounded-coords
branch
from
fbf3bf3 to
7deefda
Compare
| # Express main dims_to_collapse as non-negative integers | ||
| # and add the last (bounds specific) dimension. | ||
| dims_to_collapse = tuple( | ||
| dim % self.ndim for dim in dims_to_collapse) + (-1,) |
| self.assertArrayEqual(collapsed_coord.bounds, np.array([[-2, 82], | ||
| [8, 92], | ||
| [18, 102], | ||
| [28, 112]])) |
There was a problem hiding this comment.
Could you split this into 3 tests? i.e. one test for each collapse operation.
My concern with having them all in the one test is that if the first one fails we won't know the result of the other two collapses until we get the first one working.
I realise test_numeric_nd() that already exists in this file does the same thing as you have done here, but I do think it's better practice to split the tests up
There was a problem hiding this comment.
And perhaps add a test where the dimensions given are negative to test that handling is correct:
i.e.
collapsed_coord = coord.collapsed(-1)
There was a problem hiding this comment.
Will do, and thanks for explaining why - it makes sense. Should I do the same with the lazy equivalent below?
There was a problem hiding this comment.
Should I do the same with the lazy equivalent below?
Yes please!
There was a problem hiding this comment.
Done. Also moved the dask import to the top of the module. This is consistent with other test modules that use dask.
Since I now have self.setupTestArrays((3, 4)) in 8 of the tests, I wondered about adding a setUp method to contain that. But there are a lot of tests on the class that don't need these arrays so not really sure what the better approach is.
rcomer
force-pushed
the
partial-collapse-bounded-coords
branch
from
43d0649 to
a1470f9
Compare
3 participants
Proposed fix for #3298.