-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Do not convert subclasses of ndarray
unless required
#1118
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 | ||||
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 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. 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) | ||||
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. Need to update the message here. |
||||
|
||||
@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))) | ||||
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. Please run a PEP8 check on this code -- we try to stick to the 80 character line width and use spaces after commas. 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. |
||||
|
||||
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) | ||||
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 we adjust things so that 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. Fine, but I am not so happy about this. 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 elaborate? :) 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. The question is, what should be the exact guarantee that 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. dask arrays are not ndarray subclasses, so agree, it's not entirely the same. Either way, some users will need to wrap their calls to Just speaking personally -- and I'm happy for other to chime in with their opinions -- I find a shortcut for 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. Ok, so if you say |
||||
|
||||
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) | ||||
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. What about arithmetic that requires broadcasting? e.g., between arrays with different dimensions and/or different dimension order? That will require ensuring that 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'll have a go. |
||||
|
||||
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. | ||||
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 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. 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. 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. 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 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]) | ||||
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. Does this indicate that indexing does not preserve units? I think we should be able to fix that, see 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. Apparently yes. I didn't get around to actually look into this. |
||||
|
||||
@unittest.expectedFailure | ||||
def test_mean(self): | ||||
self.assertEqualWUnits(self.da.mean('x').values,self.v.mean(0)) | ||||
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. Let's add a comment here noting that xarray uses a wrapped version of 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. Ok 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. Actually, I don't fully understand this yet: Bare 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. Oh, interesting -- I did not realize that Quantities worked with We handle our wrapping of This dispatches to Line 325 in 6c90583
|
||||
|
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