issue_comments

ahh, now this python cls = staticmethod(lambda *args: Variable(*args).chunk()) makes a lot more sense, I didn't understand the purpose of VariableSubclassobjects before. Yes, that may potentially save us some code. I'll try it out and submit a PR for Variable

We didn't go back & forth re fixtures yet. I'll also investigate the overlap, but I think it's only partial.

Overall the tests are great, and the breadth of coverage is impressive. That's more important than their form!

The way I was thinking about leveraging existing tests is that there are a) some tests that test existing functions at least run on pint-backed arrays and b) some tests that test whether the units work correctly when used in xarray

Any opportunities to use existing code would be on (a). In the above linked Variable tests, we re-run all the tests for a dask-backed Variable by inheriting from the test class, and xfail those that don't work. (though sounds like you think that wouldn't work in this case?)

We could also try to use helper functions for data creation, but while that reduces the code it also makes understanding it a little bit harder.

Yes, there's some repetition. Did we go back & forth before re putting some of the duplicated setup in fixtures? That could cut down some boilerplate if there's a lot of overlap (though if there's only partial overlap, also increase complication, as you point out)

hmm... well, I certainly agree the tests are often quite verbose (maybe too verbose) and sometimes also test functionality of pint (e.g. when incompatible, compatible and identical units are tried). I didn't check, but I don't remember any overlaps with tests from test_dataarray.py or test_dataset.py (if that's what you meant).

To reduce the code, it might be worth to only test compatible units. We could also try to use helper functions for data creation, but while that reduces the code it also makes understanding it a little bit harder.

If Variable is part of the external API it definitely needs tests.

Reusing tests from test_dataarray.py / test_dataset.py / test_variable.py is tempting, but I don't think it is possible unless we rewrite them. Am I missing something?

I was predominately suggesting that as a way of saving your time & code on the margin (test_units.py is 4330 LOC!), and it seems like there's some overlap in code that's testing whether functions work at all, before whether the units are working correctly (though agree there's a Variable / DataArray distinction).

As from any time or code savings, I think that it's not strictly necessary to test Variable separately from Dataset & DataArray—it is implicit but it's also the external API—what do others think?

thanks for reviewing, @max-sixty

I did not plan to add tests for Variable, but that might be an oversight on my part: I have been writing tests for the functions / methods in api.rst where Variable is listed as part of the Advanced API, which means I ignored it. Also, if my understanding of xarray's internals is correct, Variable is used in both DataArray and Dataset operations, so the tests on these should implicitly also test Variable. Considering that implicit is usually not a good idea, should I add tests for Variable?

I don't think inheritance will help much, but that example could definitely be used as a reference / inspiration.

While it won't cover all the use cases, check out https://github.com/pydata/xarray/blob/master/xarray/tests/test_variable.py#L1819 when you get a chance; it's possible that inheriting from that test with a pint array might give you some tests for free

Thanks a lot as ever @keewis !

this should be ready for review and merge, unless anyone wants tests for auto_combine or map_blocks? I skipped auto_combine because it is listed as deprecated, and map_blocks because until now I avoided testing pint+dask (if I understand it correctly, map_blocks without dask is apply_ufunc?).

I will add tests for Dataset.broadcast_like and DataArray.broadcast_like in a new PR and then go over all tests up until now to fix any style inconsistencies and obvious errors. After that, I guess we have to wait on pint?

Edit: the failing distributed test on MacOSX as well as the timeout for the docs should be unrelated, I think