Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dask merge concat fill value #2520

Merged
merged 7 commits into from
May 4, 2017
Merged

Dask merge concat fill value #2520

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
74e8290
Fix concatenate fill value.
bjlittle May 3, 2017
703a49a
Fix merge fill value.
bjlittle May 3, 2017
f6a46aa
Added merge invariant tests.
bjlittle May 3, 2017
fb0a3a5
Added concatenate invariant tests.
bjlittle May 3, 2017
e222205
Review actions.
bjlittle May 4, 2017
d44458d
Simplify merge/concatenate match condition.
bjlittle May 4, 2017
414c362
expected_fill_value
bjlittle May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions lib/iris/_concatenate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,6 @@ def match(self, other, error_on_mismatch):
- scalar coords
- attributes
- dtype
- fill_value

Args:

Expand Down Expand Up @@ -465,14 +464,6 @@ def match(self, other, error_on_mismatch):
msgs.append(msg_template.format('Data types', '',
self.data_type, other.data_type))

# Check fill value.
if self.fill_value != other.fill_value:
# Allow fill-value promotion if either fill-value is None.
if self.fill_value is not None and other.fill_value is not None:
msgs.append(msg_template.format('Fill values', '',
self.fill_value,
other.fill_value))

# Check _cell_measures_and_dims
if self.cell_measures_and_dims != other.cell_measures_and_dims:
msgs.append(msg_template.format('CellMeasures', '',
Expand Down Expand Up @@ -727,10 +718,14 @@ def register(self, cube, axis=None, error_on_mismatch=False,

# Check for compatible coordinate signatures.
if match:
if self._cube_signature.fill_value is None and \
cube_signature.fill_value is not None:
# Perform proto-cube fill-value promotion.
self._cube_signature.fill_value = cube_signature.fill_value
fill_value = self._cube_signature.fill_value
# Determine whether the fill value requires to be
# demoted to the default value.
if fill_value is not None:
if cube_signature.fill_value is None or \
cube_signature.fill_value != fill_value:
# Demote the fill value to the default.
self._cube_signature.fill_value = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to

if cube_signature.fill_value != self._cube_signature.fill_value:
    self._cube_signature.fill_value = None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, also applies to _merge.py 👍

coord_signature = _CoordSignature(cube_signature)
candidate_axis = self._coord_signature.candidate_axis(
coord_signature)
Expand Down
37 changes: 19 additions & 18 deletions lib/iris/_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,11 +407,6 @@ def match(self, other, error_on_mismatch):
if self.data_type != other.data_type:
msg = 'cube data dtype differs: {} != {}'
msgs.append(msg.format(self.data_type, other.data_type))
if self.fill_value != other.fill_value:
# Allow fill-value promotion if either fill-value is None.
if self.fill_value is not None and other.fill_value is not None:
msg = 'cube fill value differs: {} != {}'
msgs.append(msg.format(self.fill_value, other.fill_value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose to allow for merging and concatenating cubes with different fill values?
I realise this is not a simple question so I can call you to discuss this if that's easier?

Copy link
Member Author

@bjlittle bjlittle May 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbdreyer This is more in alignment with the behaviour of upstream/master, which doesn't discriminate based on fill_value. So no breaking or surprising behaviour for the end user.

It also aligns with the behaviour of numpy, which will merge/concatenate arrays with different fill_values, but the resultant array has a fill_value that is replaced with the numpy nominated default for that dtype.

With regards to cubes, we're doing the same now in this PR - if we have two cubes that only differ on fill_value, say one is 1234 and the other is 4321, then they will merge/concatenate into one cube, but the resultant cube will have a cube.fill_value of None. When the user does a cube.data to get the data, the resulting masked array (assuming the payload realizes as masked) will have a fill_value that is the numpy nominated default - note that, the cube.fill_value remains equal to None in this case ... until the user changes it via cube.fill_value = fill_value, or cube.replace(..., fill_value=fill_value)

if (self.cell_measures_and_dims != other.cell_measures_and_dims):
msgs.append('cube.cell_measures differ')

Expand Down Expand Up @@ -1285,15 +1280,19 @@ def register(self, cube, error_on_mismatch=False):
this :class:`ProtoCube`.

"""
signature = self._cube_signature
cube_signature = self._cube_signature
other = self._build_signature(cube)
match = signature.match(other, error_on_mismatch)
match = cube_signature.match(other, error_on_mismatch)
if match:
if signature.fill_value is None and other.fill_value is not None:
# Perform proto-cube fill-value promotion.
fv = other.fill_value
self._cube_signature = self._build_signature(self._source,
fill_value=fv)
# Determine whether the fill value requires to be demoted
# to the default value.
if cube_signature.fill_value is not None:
if other.fill_value is None or \
cube_signature.fill_value != other.fill_value:
# Demote the fill value to the default.
signature = self._build_signature(self._source,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a call to _build_signature here, but in the concatenate case you can just set fill_value to None?

Copy link
Member Author

@bjlittle bjlittle May 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One is a class instance (writable) the other is a specialization of a namedtuple (read-only)

default_fill_value=True)
self._cube_signature = signature
coord_payload = self._extract_coord_payload(cube)
match = coord_payload.match_signature(self._coord_signature,
error_on_mismatch)
Expand Down Expand Up @@ -1614,7 +1613,7 @@ def _build_coordinates(self):
self._vector_aux_coords_dims):
aux_coords_and_dims.append(_CoordAndDims(item.coord, dims))

def _build_signature(self, cube, fill_value=None):
def _build_signature(self, cube, default_fill_value=False):
"""
Generate the signature that defines this cube.

Expand All @@ -1625,16 +1624,18 @@ def _build_signature(self, cube, fill_value=None):

Kwargs:

* fill_value:
Promoted fill-value to use instead of the source cube
fill-value.
* default_fill_value:
Override the cube fill value with the default fill value of None.
Default is False i.e. use the provided cube.fill_value when
constructing the cube signature.

Returns:
The cube signature.

"""
if fill_value is None:
fill_value = cube.fill_value
fill_value = cube.fill_value
if default_fill_value:
fill_value = None
return _CubeSignature(cube.metadata, cube.shape, cube.dtype,
fill_value, cube._cell_measures_and_dims)

Expand Down
2 changes: 1 addition & 1 deletion lib/iris/tests/results/concatenate/concat_2y2d.cml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" ?>
<cubes xmlns="urn:x-iris:cubeml-0.2">
<cube core-dtype="float32" dtype="float32" units="unknown">
<cube core-dtype="float32" dtype="float32" fill_value="1234.0" units="unknown">
<coords>
<coord datadims="[1]">
<dimCoord bounds="[[-0.5, 0.5],
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/tests/results/concatenate/concat_masked_2x2d.cml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" ?>
<cubes xmlns="urn:x-iris:cubeml-0.2">
<cube core-dtype="float32" dtype="float32" fill_value="1234.0" units="unknown">
<cube core-dtype="float32" dtype="float32" units="unknown">
<coords>
<coord datadims="[1]">
<dimCoord bounds="[[-0.5, 0.5],
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/tests/results/concatenate/concat_masked_2y2d.cml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" ?>
<cubes xmlns="urn:x-iris:cubeml-0.2">
<cube core-dtype="float32" dtype="float32" fill_value="1234.0" units="unknown">
<cube core-dtype="float32" dtype="float32" units="unknown">
<coords>
<coord datadims="[1]">
<dimCoord bounds="[[-0.5, 0.5],
Expand Down
Loading