-
Notifications
You must be signed in to change notification settings - Fork 300
fill_value and dtype #2433
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
fill_value and dtype #2433
Changes from all commits
42074db
bb972e5
338f8d9
0b80591
db1f7e1
60344ca
8686da5
de1f83e
6bf80a7
e0b5609
9b6636d
385cdb9
cf2af09
445b4e6
2b77106
479dad9
cc14ab7
b27917c
0a80c37
f4fd7a0
2306a86
d3b4212
c74dc10
149a3be
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 |
|---|---|---|
|
|
@@ -65,8 +65,8 @@ class CubeMetadata(collections.namedtuple('CubeMetadata', | |
| 'units', | ||
| 'attributes', | ||
| 'cell_methods', | ||
| 'fill_value', | ||
| 'dtype'])): | ||
| 'dtype', | ||
| 'fill_value'])): | ||
| """ | ||
| Represents the phenomenon metadata for a single :class:`Cube`. | ||
|
|
||
|
|
@@ -751,10 +751,12 @@ def __init__(self, data, standard_name=None, long_name=None, | |
| # Cell Measures | ||
| self._cell_measures_and_dims = [] | ||
|
|
||
| # We need to set the dtype before the fill_value, | ||
| # as the fill_value is checked against self.dtype. | ||
| self._dtype = None | ||
| self.dtype = dtype | ||
| self.fill_value = fill_value | ||
|
|
||
| self._dtype = dtype | ||
|
|
||
| identities = set() | ||
| if dim_coords_and_dims: | ||
| dims = set() | ||
|
|
@@ -798,7 +800,7 @@ def metadata(self): | |
| """ | ||
| return CubeMetadata(self.standard_name, self.long_name, self.var_name, | ||
| self.units, self.attributes, self.cell_methods, | ||
| self.fill_value, self._dtype) | ||
| self._dtype, self.fill_value) | ||
|
|
||
| @metadata.setter | ||
| def metadata(self, value): | ||
|
|
@@ -813,7 +815,10 @@ def metadata(self, value): | |
| if missing_attrs: | ||
| raise TypeError('Invalid/incomplete metadata') | ||
| for name in CubeMetadata._fields: | ||
| setattr(self, name, getattr(value, name)) | ||
| alias = name | ||
| if name in ['dtype', 'fill_value']: | ||
| alias = '_{}'.format(name) | ||
| setattr(self, alias, getattr(value, name)) | ||
|
|
||
| def is_compatible(self, other, ignore=None): | ||
| """ | ||
|
|
@@ -1632,7 +1637,38 @@ def dtype(self): | |
|
|
||
| @dtype.setter | ||
| def dtype(self, dtype): | ||
| self._dtype = dtype | ||
| if dtype != self.dtype: | ||
|
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. Should we not always allow
Member
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 can see the case that you might want to unset the dtype for lazy data to |
||
| if dtype is not None: | ||
| if not self.has_lazy_data(): | ||
| emsg = 'Cube does not have lazy data, cannot set dtype.' | ||
| raise ValueError(emsg) | ||
| dtype = np.dtype(dtype) | ||
| if dtype.kind != 'i': | ||
| emsg = ('Can only cast lazy data to integral dtype, ' | ||
| 'got {!r}.') | ||
|
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. Why?
Member
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. We need to be strict here. We're only allowing the If we do want to support casting of data, for the sake of it, then we should support that properly, say with
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. Just a note: if you're visiting this bit, I've found that this test should also be allowing 'u' type data, i.e. unsigned ints. E.G.
Member
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. Hmmm good point @pp-mo ... See this concatenate change and here also ... is there a generic numpy kind to catch all integral kinds instead of
Member
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. This numpy scalars link was handy ... also see Array-protocol type strings section of numpy docs ... And this pattern seems to work: >>> integral = [np.arange(10, dtype=np.int8),
... np.arange(10, dtype=np.int16),
... np.arange(10, dtype=np.int32),
... np.arange(10, dtype=np.int64),
... np.arange(10, dtype=np.uint8),
... np.arange(10, dtype=np.uint16),
... np.arange(10, dtype=np.uint32),
... np.arange(10, dtype=np.uint64)]
>>> for i in integral:
... print(isinstance(i[0], np.integer))
...
True
True
True
True
True
True
True
True
>>> f = np.arange(10, dtype=np.float)
>>> isinstance(f[0], np.integer)
FalseDon't know if it's preferable over
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.
It might be worth putting a comment in the code about 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. I only mentioned the kind=u/i thing for completeness..
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. I do prefer np.integer.
Member
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. @pp-mo can you make an issue for this please |
||
| raise ValueError(emsg.format(dtype)) | ||
| self._fill_value = None | ||
| self._dtype = dtype | ||
|
|
||
| @property | ||
| def fill_value(self): | ||
| return self._fill_value | ||
|
|
||
| @fill_value.setter | ||
| def fill_value(self, fill_value): | ||
| if fill_value is not None: | ||
| # Convert the given value to the dtype of the cube. | ||
| fill_value = np.asarray([fill_value])[0] | ||
|
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. How about
Member
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'm tending to steer away from the However, we could opt for
Member
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'll make this change ... |
||
| target_dtype = self.dtype | ||
| if fill_value.dtype.kind == 'f' and target_dtype.kind == 'i': | ||
| # Perform rounding when converting floats to ints. | ||
| fill_value = np.rint(fill_value) | ||
| try: | ||
| [fill_value] = np.asarray([fill_value], dtype=target_dtype) | ||
| except OverflowError: | ||
| emsg = 'Fill value of {!r} invalid for cube {!r}.' | ||
| raise ValueError(emsg.format(fill_value, self.dtype)) | ||
| self._fill_value = fill_value | ||
|
|
||
| @property | ||
| def ndim(self): | ||
|
|
@@ -1731,12 +1767,17 @@ def data(self, value): | |
| raise ValueError('Require cube data with shape %r, got ' | ||
| '%r.' % (self.shape, value.shape)) | ||
|
|
||
| # Set lazy or real data, and reset the other. | ||
| if is_lazy_data(value): | ||
| self._dask_array = value | ||
| self._numpy_array = None | ||
|
|
||
| else: | ||
| self._numpy_array = value | ||
| self._dask_array = None | ||
|
|
||
| # Cancel any 'realisation' datatype conversion, and fill value. | ||
| self.dtype = None | ||
| self.fill_value = None | ||
|
|
||
| def has_lazy_data(self): | ||
| return self._numpy_array is None | ||
|
|
||
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.
I should have some unit tests for this really ...
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.
Actually, I'm not going to do that here as this might all change given conversations with @pp-mo
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.
Created the following issue #2439 for this