issue_comments

Thanks @keewis. I'm really excited to see this go in.

Once the CI passes this should be ready for review and merging.

I tried that by writing some sort of assert_duckarray_equal and assert_duckarray_allclose function. That should work for most duck arrays, but not pint: the tests often compare the data with the variable (or the variable's data), and since the data is mostly a bare numpy array this fails for pint: bare numpy arrays are by definition dimensionless and so the comparison either returns False (the assertion fails) or a DimensionalityError is raised.

So to make this work I think we'd need to rewrite VariableSubtests to have another static method (array_type? data_type?) which is called on all data before it is passed to cls. That, however, has its own set of issues and in the end I concluded that VariableSubtests was just not written with duck arrays in mind, and that pytest doesn't encourage reusing tests by using inheritance (marking or removing tests from base classes is really clunky). With that in mind I simply removed the use of VariableSubtests for now (this shouldn't decrease the coverage too much) and we can add it back if / when we notice that we actually need these tests.

Edit: Right now the only failing tests are due to hgrecco/pint#1123. I guess I'll just remove the xfail condition and add it back once that PR is merged.

I just saw your message from a couple of days ago @keewis . To the extent you've solved that, great. If you haven't — one option would be to change assert_array_equal in those tests to xarray's assert_equal, and ensure xarray's functions handle the duck arrays reasonably.

unfortunately, this doesn't seem to really work: VariableSubtests was written to use the same tests for Variable and IndexVariable, and using it to test duckarrays (pint in particular) is difficult since the data that is often used to compare the result is not converted. I'm not sure if rewriting VariableSubtests to optionally allow converting the data or adding specific Variable-with-units tests instead of inheriting from VariableSubtests is easier...

I think we could also write our own version of assert_array_equal / assert_allclose that works on duck arrays.

This sounds right to me.

except from the failing operator.eq tests (which are due to pint upstream and will be fixed soon) and the pad tests that need to be rewritten only the tests from the VariableSubclassobjects are left.

Since they use assert_array_equal and np.testing.assert_allclose (which don't dispatch), I'm not sure how to deal with them: If I wanted to fix them in TestVariable I would probably end up rewriting all of these. Other than that, I think we could also write our own version of assert_array_equal / assert_allclose that works on duck arrays. We already have the functionality in duck_array_ops.py, we'd just need to write the summary code. That would probably have to be a new PR, though.