-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Do not convert subclasses of ndarray unless required
#1118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
40df7b0
37b484c
921915e
b3a49b6
28ef399
c849b67
39dedb1
c58f92e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||
| import numpy as np | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a docstring explaining that this module uses quantities as a concrete example, but really it's just checking for subclass compatibility. Also, I might call it something more genericl ike
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I was rather thinking of this test-file as specialised towards physical units handling with quantities (i.e. as a regression-test database geared towards #525). I'd rather make a separate test-case for general subclasses, maybe with a dummy subclass.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, |
||||
| import pandas as pd | ||||
|
|
||||
| from xarray import (align, broadcast, Dataset, DataArray, Variable) | ||||
|
|
||||
| from xarray.test import (TestCase, unittest) | ||||
|
|
||||
| try: | ||||
| import quantities as pq | ||||
| has_quantities = True | ||||
| except ImportError: | ||||
| has_quantities = False | ||||
|
|
||||
| def requires_quantities(test): | ||||
| return test if has_quantities else unittest.skip('requires dask')(test) | ||||
|
||||
|
|
||||
| @requires_quantities | ||||
| class TestWithQuantities(TestCase): | ||||
| def setUp(self): | ||||
| self.x = np.arange(10) * pq.A | ||||
| self.y = np.arange(20) | ||||
| self.xp = np.arange(10) * pq.J | ||||
| self.v = np.arange(10*20).reshape(10,20) * pq.V | ||||
| self.da = DataArray(self.v,dims=['x','y'],coords=dict(x=self.x,y=self.y,xp=(['x'],self.xp))) | ||||
|
||||
|
|
||||
| def assertEqualWUnits(self,a,b): | ||||
| self.assertIsNotNone(getattr(a, 'units', None)) | ||||
| self.assertIsNotNone(getattr(b, 'units', None)) | ||||
| self.assertEqual(a.units,b.units) | ||||
| np.testing.assert_allclose(a.magnitude,b.magnitude) | ||||
|
|
||||
| def test_units_in_data_and_coords(self): | ||||
| da = self.da | ||||
| self.assertEqualWUnits(da.xp.data,self.xp) | ||||
| self.assertEqualWUnits(da.data,self.v) | ||||
|
||||
|
|
||||
| def test_arithmetics(self): | ||||
| da = self.da | ||||
| f = DataArray(np.arange(10*20).reshape(10,20)*pq.A,dims=['x','y'],coords=dict(x=self.x,y=self.y)) | ||||
| self.assertEqualWUnits((da*f).data, da.data*f.data) | ||||
|
||||
|
|
||||
| def test_unit_checking(self): | ||||
| da = self.da | ||||
| f = DataArray(np.arange(10*20).reshape(10,20)*pq.A,dims=['x','y'],coords=dict(x=self.x,y=self.y)) | ||||
| with self.assertRaisesRegex(ValueError,'Unable to convert between units'): | ||||
| da + f | ||||
|
|
||||
| @unittest.expectedFailure | ||||
| def test_units_in_indexes(self): | ||||
| """ | ||||
| Indexes are borrowed from Pandas, and Pandas does not support units. | ||||
| Therefore, we currently don't intend to support units on indexes either. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usually we use comments instead of docstrings on test methods
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? I do not see how docstrings could be worse than comments under any circumstance... On the other hand, docstrings allow for introspection from unit-testing tools. But then, I don't really care.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, not important, but I'm really surprised by this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the convention for NumPy. To be honest I'm not sure why, but I recall being told so at one point by @charris. I see now that this isn't followed for pandas, so I guess we can break it here. I don't feel too strongly either way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remember well, the test docstrings were changing the way
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pytest doesn't seem to with either |
||||
| """ | ||||
| da = self.da | ||||
| self.assertEqualWUnits(da.x.data,self.x) | ||||
|
|
||||
| @unittest.expectedFailure | ||||
| def test_sel(self): | ||||
| self.assertEqualWUnits(self.da.sel(y=self.y[0]).values,self.v[:,0]) | ||||
|
||||
|
|
||||
| @unittest.expectedFailure | ||||
| def test_mean(self): | ||||
| self.assertEqualWUnits(self.da.mean('x').values,self.v.mean(0)) | ||||
|
||||
| if isinstance(axis, tuple) or not values.dtype.isnative or no_bottleneck: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should probably be renamed, e.g.,
_as_any_array_or_item, just to make it super clear what it's doing (also update the TODO in the docstring)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok